Conversation
…Responses router (R3)
Rich content parts landed in P1 (`ResponseContentPart::{InputImage, InputFile, Refusal}`).
R1 wired them at the OpenAI-compat router via pure passthrough. R2 (parallel) wires them
at the gRPC regular Responses router by reusing the Chat Completions multimodal pipeline.
R3 (this change) wires them at the gRPC harmony Responses router.
The harmony path is structurally different from both R1 and R2: gpt-oss and other
harmony-target models have no multimodal wire to the backend. The chat harmony
pipeline in `harmony/stages/request_building.rs` explicitly passes `None` for
multimodal on every backend (sglang/vllm/trtllm). Reusing `grpc/multimodal.rs` at
the Responses layer without threading bytes through
`build_generate_request_from_responses` and the harmony prompt builder would produce
a prompt that advertises an image the backend cannot actually see.
Rather than inventing a new harmony-multimodal protocol (out of scope for a content-part
transformer task), R3 takes the honest path: upgrade the pre-R3 silent drop of
image/file parts in `harmony/builder.rs` into an explicit 400 `unsupported_content`
at the router entry point. Silent data loss on multimodal prompts is the worst kind
of failure mode — the caller believes the model saw their image, the model responds
from text context alone, and the discrepancy is invisible at every observable layer.
What changed
- `model_gateway/src/routers/grpc/harmony/responses/content_parts.rs` (new): single
source of truth for the rejection policy. Walks `ResponseInput::Items`,
inspects `ResponseInputOutputItem::Message` and `SimpleInputMessage` bodies, and
rejects with a 400 `unsupported_content` on:
* `InputImage` with any combination of `file_id`, `image_url` (absolute or
`data:` URL). Distinct messages identify the specific gap.
* `InputFile` with any combination of `file_data`, `file_id`, `file_url`.
* `InputFile` `file_data` whose base64-decoded prefix starts with `%PDF-`
returns an R4-specific message ("PDF extraction pending R4") so callers can
correlate the gap with the paused task. Robust against
`data:application/pdf;base64,` wrappers inside the `file_data` field.
* `Refusal` on any non-assistant role. Assistant-role refusals remain accepted
so multi-turn replay of a prior model refusal keeps working.
- `model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs`: invoke
`validate_harmony_responses_input` as the first action in `serve_harmony_responses`,
before `load_previous_messages` merges persisted history — keeps stored
assistant-refusal replays untouched while surfacing fresh-input issues up-front.
- `model_gateway/src/routers/grpc/harmony/responses/streaming.rs`: mirror the
validation in `serve_harmony_responses_stream` before history load and SSE channel
setup so the client sees a plain HTTP 400 instead of an SSE stream that
immediately closes.
- `model_gateway/src/routers/grpc/harmony/responses/mod.rs`: declare the new
`content_parts` module.
- `model_gateway/src/routers/grpc/harmony/builder.rs`: refresh the two
`parse_response_item_to_harmony_message` / `SimpleInputMessage` comments that
still referenced the open R1/R2/R3 task. The `None` arms for
`InputImage`/`InputFile` remain as a defensive fallback for any future caller
that bypasses the validator — document that they are unreachable in practice.
Why
- Spec-parity with P1/R1/R2 on the harmony surface.
- Eliminates a silent-drop failure mode that survives through the end of the
request lifecycle.
- Surfaces the pending R4 PDF gap with a specific error message so clients know
the rejection is tracked rather than a generic unsupported-content case.
How
- Validation is a pure function operating on `&ResponsesRequest`; rejection
returns `Result<(), Response>` so entry-point wiring is a one-line `?` or
`if let Err(...)` pattern.
- Magic-byte sniffing on `file_data` uses the base64 0.22 engine already on the
workspace; no new dependency. Strips optional `data:<mime>;base64,` wrappers.
- Refusal role gating: only `role == "assistant"` allows refusal content parts
through, matching the P2 distinction between tagged Message (user/system/
developer) and assistant replay (P2 §Pass 2 Drift 2).
PDF text extraction remains out of scope for R3 (tracked as R4, paused on
`pdf-extract` dep approval). The R4 message is emitted here so callers correlate
the gap correctly; the actual extractor is not added.
Harmony-template integration note
- Image threading: not wired. gpt-oss was trained on `<|message|>` text channels
only; no image-token prefix or separate tensor field is read by harmony
backends today. When/if multimodal harmony support lands (separate task), the
validator here should be relaxed and a real `multimodal` parameter threaded
through `harmony/stages/request_building.rs` → `build_generate_request_from_responses`
on each backend.
Tests
- `content_parts::tests` covers the full taxonomy (14 tests): accepted text-only
paths (pure-string, InputText, SimpleInputMessage.String, assistant-role
Refusal replay) and every rejected branch (InputImage data-URL/absolute/file_id;
InputFile file_data PDF/JPEG, file_url, file_id; non-assistant Refusal;
SimpleInputMessage.Array image; data-URL-wrapped PDF file_data).
- All 684 lib tests + other test binaries pass.
- `cargo clippy -p smg --all-targets -- -D warnings` clean.
Unblocks R5 (harmony row), E1 (harmony file-input row), and E2 (harmony
image-input row) — each of which can now assert the expected 400 response on
their harmony matrix cells.
Refs: .claude/_audit/responses-api-gap-audit.md §Task R3
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Harmony Responses (R3) input validator that rejects unsupported content parts (images, files, refusals on non‑assistant roles) at router entry and after history merge; documents defensive handling in the Harmony builder and integrates validation into both streaming and non‑streaming handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router/Handler
participant Validator as Validator (content_parts)
participant History as History Loader
participant Builder as Builder
participant Harmony as Harmony Backend
Client->>Router: ResponsesRequest
Router->>Validator: validate_harmony_responses_input(request)
alt validation fails
Validator-->>Router: Error (400 unsupported_content)
Router-->>Client: HTTP error response
else validation passes
Router->>History: load_previous_messages(...)
History-->>Router: merged messages
Router->>Validator: validate_harmony_responses_input(current_request)
alt validation fails
Validator-->>Router: Error (400 unsupported_content)
Router-->>Client: HTTP error response
else
Router->>Builder: parse_response_items(...)
Builder->>Harmony: send Harmony message(s)
Harmony-->>Builder: Harmony response
Builder-->>Router: formatted response
Router-->>Client: Success (streamed or non-streamed)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces explicit validation for content parts within the Harmony Responses pipeline to prevent silent data loss. A new content_parts module was added to reject unsupported items such as images, files, and non-assistant refusals with a 400 error, and this validation has been integrated into both streaming and non-streaming entry points. Feedback was provided to optimize the PDF detection logic by using a fixed-size buffer for base64 decoding instead of allocating memory for the entire payload.
| fn looks_like_pdf(file_data: &str) -> bool { | ||
| let payload = strip_data_url_prefix(file_data); | ||
| let Ok(decoded) = BASE64_STANDARD.decode(payload.trim()) else { | ||
| // If base64 fails we fall through to the generic | ||
| // "file_data unsupported" message rather than a PDF-specific | ||
| // one; malformed base64 is a separate kind of error and the | ||
| // harmony backend can't handle it anyway. | ||
| return false; | ||
| }; | ||
| decoded.starts_with(PDF_MAGIC) | ||
| } |
There was a problem hiding this comment.
The current implementation of looks_like_pdf decodes the entire file_data payload into a new Vec<u8> just to check the first 5 bytes for the PDF magic number. For large file uploads, this results in unnecessary memory allocation. Additionally, to reliably distinguish data URIs, you should parse the URL and check its scheme instead of using string prefix matching. You should then decode only a small prefix of the base64 string using a fixed-size buffer.
fn looks_like_pdf(file_data: &str) -> bool {
let Ok(url) = url::Url::parse(file_data) else {
return false;
};
if url.scheme() != "data" {
return false;
}
let path = url.path();
let Some((_, payload)) = path.split_once(',') else {
return false;
};
let payload = payload.trim();
let prefix_len = (payload.len().min(32) / 4) * 4;
if prefix_len == 0 {
return false;
}
let Some(prefix) = payload.get(..prefix_len) else {
return false;
};
let mut buf = [0u8; 32];
if let Ok(len) = BASE64_STANDARD.decode_slice(prefix.as_bytes(), &mut buf) {
return buf[..len].starts_with(PDF_MAGIC);
}
false
}References
- To reliably distinguish data URIs from other URLs, parse the URL and check its scheme instead of using string prefix matching. This avoids misclassifying relative URLs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| match input.find(";base64,") { | ||
| Some(idx) => &input[idx + ";base64,".len()..], |
There was a problem hiding this comment.
🟡 Nit: is_data_url matches data: case-insensitively (per RFC 2397), but strip_data_url_prefix uses a case-sensitive find(";base64,"). A client sending data:application/pdf;Base64,JVBERi... would pass the is_data_url gate but miss the strip, causing looks_like_pdf to fail the base64 decode and return false. The request is still correctly rejected — just with the generic "file_data unsupported" message instead of the PDF-specific R4 message.
Not a correctness bug since the rejection still happens, but worth aligning the case handling for consistent error messaging:
| match input.find(";base64,") { | |
| Some(idx) => &input[idx + ";base64,".len()..], | |
| match input.to_ascii_lowercase().find(";base64,") { |
(Or use input.as_bytes().windows(8) to find the offset case-insensitively without allocating.)
There was a problem hiding this comment.
Clean PR — R3 validation is well-placed at both entry points (non-streaming and streaming), the rejection policy matches the spec, and the test matrix covers every content-part variant including the SimpleInputMessage path.
Summary: 0 🔴 Important · 1 🟡 Nit · 0 🟣 Pre-existing
The nit is a minor case-sensitivity inconsistency in the data-URL prefix stripper that could affect which error message a PDF upload gets, but does not affect correctness (the request is still rejected).
![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: bb442a8aaf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// clients that send the latter in a `file_data` field. | ||
| fn looks_like_pdf(file_data: &str) -> bool { | ||
| let payload = strip_data_url_prefix(file_data); | ||
| let Ok(decoded) = BASE64_STANDARD.decode(payload.trim()) else { |
There was a problem hiding this comment.
looks_like_pdf currently base64-decodes the entire file_data string just to check for %PDF-, which can allocate and CPU-burn on very large inputs before we return the expected 400. With the router default max_payload_size set to 512MB (model_gateway/src/config/types.rs), a single rejected input_file.file_data request can trigger hundreds of MB of extra allocation on the request path, making this endpoint much easier to exhaust under load. Decode only the minimal prefix needed to check the magic bytes instead of materializing the whole payload.
Useful? React with 👍 / 👎.
github-actions
Bot
added
grpc
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/harmony/responses/content_parts.rs`:
- Around line 227-236: The looks_like_pdf function currently calls
BASE64_STANDARD.decode on the entire payload which materializes the full
attachment; instead, only decode a small base64 prefix sufficient to produce the
first PDF_MAGIC bytes (or use a streaming/base64 decoder). Trim the data URL
with strip_data_url_prefix(payload), then compute how many base64 characters are
needed to yield PDF_MAGIC.len() bytes (account for base64 padding), take that
prefix (plus a bit of extra to be safe), decode only that prefix with
BASE64_STANDARD.decode (or feed it to a streaming decoder) and then check
decoded.starts_with(PDF_MAGIC); ensure you still return false on decode errors
and preserve the existing error semantics.
- Around line 74-78: The helper currently only validates caller-submitted
request.input and misses previously persisted history, so re-run the same
validation against the merged request after load_previous_messages: after
calling load_previous_messages (and before handing the merged messages to the
builder/resume path), scan the merged history slice for InputImage/InputFile
parts (but still allow assistant-role refusals) and reject or surface invalid
parts the same way you do for request.input; ensure this additional check uses
the same validation logic/path as the existing helper so threads with pre-R3
history are caught (apply same change to the analogous check referenced around
the second block).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0234888a-c99c-4cfd-bf60-82842baa6c50
📒 Files selected for processing (5)
model_gateway/src/routers/grpc/harmony/builder.rsmodel_gateway/src/routers/grpc/harmony/responses/content_parts.rsmodel_gateway/src/routers/grpc/harmony/responses/mod.rsmodel_gateway/src/routers/grpc/harmony/responses/non_streaming.rsmodel_gateway/src/routers/grpc/harmony/responses/streaming.rs
…r case
Address three R3 review nits (gemini-code-assist, claude, chatgpt-codex):
1. **DOS-adjacent allocation on large `file_data` rejections** (codex P2, gemini
medium): `looks_like_pdf` previously decoded the *entire* `file_data` base64
payload into a `Vec<u8>` just to check its first 5 bytes. With the router's
default `max_payload_size` of 512 MB (model_gateway/src/config/types.rs), a
single rejected `input_file.file_data` request could trigger hundreds of MB
of allocation on the request path — turning R3's 400-fast-path into a
resource exhaustion vector under load.
Rewrite `looks_like_pdf` to decode only the first 8 base64 characters (6
bytes) into a fixed-size stack buffer via `BASE64_STANDARD.decode_slice`.
That window is the smallest multiple-of-4 that still covers the 5-byte
`%PDF-` magic. Full payload is never materialized.
2. **Case-insensitive `;base64,` separator** (claude nit): `is_data_url`
matches `data:` case-insensitively per RFC 2397, but the old
`strip_data_url_prefix` used a case-sensitive `find(";base64,")`. A client
sending `data:application/pdf;Base64,JVBERi...` would pass the data-URL
gate and then miss the strip, falling through to the generic "file_data
unsupported" branch instead of the PDF-specific R4 message. The request
was still correctly rejected, but the error code was less actionable.
Extract `find_base64_separator` using a no-allocation byte-window walk
with `eq_ignore_ascii_case`, keeping the strip path case-insensitive
end-to-end.
3. Documented both helpers with inline rationale + cross-references to the
config default and RFC. Added two new tests:
- `data_url_prefix_is_case_insensitive_on_base64_separator` locks in
the R4-message outcome on mixed-case wrappers.
- `pdf_sniff_decodes_only_magic_prefix_not_full_payload` guards the
bounded-decode path with a 64 KiB+ payload whose first 6 bytes
decode to `%PDF-1` and whose remainder is filler the sniffer must
never touch.
No behavior change for valid content-part inputs; only tightens the reject
path and improves error-message specificity. All 16 content_parts tests pass.
`cargo clippy -p smg --all-targets -- -D warnings` clean.
Refs: .claude/_audit/responses-api-gap-audit.md §Task R3
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
model_gateway/src/routers/grpc/harmony/responses/content_parts.rs (1)
74-78:⚠️ Potential issue | 🟠 MajorValidate merged history too, not only fresh
request.input.Line 74-78 explicitly skip persisted history. That leaves pre-R3 stored conversations with
InputImage/InputFileable to bypass this guard and still reach the silent-drop path on resume. Please run the same validation afterload_previous_messageson the merged input before builder dispatch.Also applies to: 84-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/harmony/responses/content_parts.rs` around lines 74 - 78, The current validation only inspects caller-submitted request.input and skips persisted history, allowing persisted InputImage/InputFile to bypass checks; after calling load_previous_messages (or wherever previous responses are merged into the request), run the same validation routine you use for request.input on the merged/combined input before dispatching to the builder (i.e., validate the merged messages returned by load_previous_messages or the variable that holds merged_input), ensuring the same guards for InputImage/InputFile and silent-drop logic are applied to persisted history as well as fresh input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/harmony/responses/content_parts.rs`:
- Around line 346-353: The test helper assert_is_unsupported currently only
checks status and HEADER_X_SMG_ERROR_CODE; update it to also read the response
body and assert it contains the R4-specific PDF rejection substring so PDF-path
tests prove message-level behavior; locate the function assert_is_unsupported
and after the existing header checks, read Response::body (or
response.text()/to_string()) and assert the body contains the R4 PDF-specific
message substring used by the service (i.e., the PDF-specific rejection text),
and apply the same change to the other similar test helpers/blocks noted (the
branches around the other ranges) so those R4-PDF tests validate the message
body as well.
---
Duplicate comments:
In `@model_gateway/src/routers/grpc/harmony/responses/content_parts.rs`:
- Around line 74-78: The current validation only inspects caller-submitted
request.input and skips persisted history, allowing persisted
InputImage/InputFile to bypass checks; after calling load_previous_messages (or
wherever previous responses are merged into the request), run the same
validation routine you use for request.input on the merged/combined input before
dispatching to the builder (i.e., validate the merged messages returned by
load_previous_messages or the variable that holds merged_input), ensuring the
same guards for InputImage/InputFile and silent-drop logic are applied to
persisted history as well as fresh input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68ffd61a-2288-48da-9959-4146002be80b
📒 Files selected for processing (1)
model_gateway/src/routers/grpc/harmony/responses/content_parts.rs
… message body
Address two CodeRabbit review findings:
1. **Major — Revalidate merged history** (coderabbitai). The R3 validator only
inspected `request.input`, so stored threads that were persisted by a sibling
router supporting multimodal (notably R1's OpenAI-compat passthrough) could
replay with `InputImage` / `InputFile` content parts on the harmony surface:
they would pass the fresh-input check, get merged by `load_previous_messages`,
and hit the exact silent-drop path this PR exists to close.
Re-run `validate_harmony_responses_input` on the merged `current_request`
in both the non-streaming and streaming entry points, after the history
merge. The validator keeps its assistant-role-refusal exception so
legitimate multi-turn output replay continues to work; only non-assistant
image/file/refusal parts in history fail the second pass.
2. **Nit — assert PDF-specific message body** (coderabbitai). The PDF-path
tests only asserted `400 unsupported_content`, which is satisfied by both
PDF-specific and generic file-data rejections. That makes the R4-message
branch untested at the message level.
Add an async `error_message` test helper that reads the JSON body and
extracts `error.message`, then upgrade four tests to assert on the body:
- `input_file_pdf_magic_bytes_are_rejected_with_r4_message` — message
contains `PDF` and `R4`.
- `input_file_non_pdf_file_data_is_rejected` — message does NOT contain
`R4` (proves the non-PDF branch stays out of the R4 path).
- `data_url_prefix_pdf_magic_still_rejected_with_r4_message` — message
contains `PDF` + `R4` after data-URL stripping.
- `data_url_prefix_is_case_insensitive_on_base64_separator` — message
contains `PDF` + `R4` after case-insensitive stripping.
- `pdf_sniff_decodes_only_magic_prefix_not_full_payload` — large payload
still reaches the R4-specific message via bounded decoding.
Also add `refusal_role_rejection_cites_assistant_exception` to lock in the
human-readable rationale the caller sees for refusal rejections.
No behavior change for valid content-part inputs; only tightens the reject-
path coverage and closes the stored-history regression. All 17 content_parts
tests pass. `cargo clippy -p smg --all-targets -- -D warnings` clean.
Refs: .claude/_audit/responses-api-gap-audit.md §Task R3
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2599f45d08
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| input | ||
| .as_bytes() | ||
| .windows(NEEDLE.len()) | ||
| .position(|window| window.eq_ignore_ascii_case(NEEDLE)) |
There was a problem hiding this comment.
When file_data begins with data: but does not contain an early ;base64, marker, find_base64_separator scans every 8-byte window across the entire string before returning None. With the router's large request-size limits, a malformed data: payload can still force a CPU-heavy linear scan on hundreds of MB even though we only need header metadata to decide PDF-sniffing behavior. Limiting the search to the data-URL header region (e.g., up to the first comma or a small prefix cap) avoids this request-path DoS vector while preserving current behavior.
Useful? React with 👍 / 👎.
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 `@model_gateway/src/routers/grpc/harmony/responses/content_parts.rs`:
- Around line 194-197: The fallback branch that returns unsupported(...) when
file_data, file_id, and file_url are all None should be annotated to clarify
intent: update the match/if branch handling InputFile so it includes a short
comment stating whether this arm is defense-in-depth (protocol layer may not
guarantee non-empty input) or is expected to be unreachable because upstream
validation ensures at least one of file_data/file_id/file_url is present;
reference the InputFile fields (file_data, file_id, file_url) and the
unsupported(...) call in your comment so future readers know why this error
remains here.
- Around line 74-78: Update the doc comment above the validator that inspects
caller-submitted request.input to reflect the actual two-phase usage: note that
this validator is invoked once on the raw request.input and invoked again after
load_previous_messages merges persisted history, so the second validation covers
the merged message slice; mention both call sites (the initial request
validation and the post-merge validation after load_previous_messages) and
clarify that persisted history is validated when merged rather than being wholly
"skipped."
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4fe7d9a-590a-4438-8b51-64006cfb88be
📒 Files selected for processing (3)
model_gateway/src/routers/grpc/harmony/responses/content_parts.rsmodel_gateway/src/routers/grpc/harmony/responses/non_streaming.rsmodel_gateway/src/routers/grpc/harmony/responses/streaming.rs
…omments Address three review comments on the R3 PR: 1. **P2 codex — bound `find_base64_separator` scan**. A `data:` payload without an early `;base64,` separator previously walked every 8-byte window across the full string before giving up. Combined with the router's large `max_payload_size` default (512 MB in `model_gateway/src/config/types.rs`), that turned the fast-path 400 rejection into an O(n) CPU walk on adversarial payloads. Cap the scan at the first 256 bytes via `DATA_URL_HEADER_SCAN_LIMIT`. RFC 2397 data-URL headers carry at most a MIME type + a handful of parameters, so 256 bytes is comfortably larger than any realistic header. Beyond the cap the sniffer falls through to the generic file-data rejection — the same behavior as malformed base64 — so no valid payload regresses. Add `data_url_separator_scan_is_bounded_to_header_region` to lock the cap in: constructs a `data:` payload with a 64 KiB header and no early separator, asserts the validator rejects without claiming PDF identification. 2. **Nit coderabbit — doc comment outdated**. The comment above `validate_harmony_responses_input` still claimed persisted history is "skipped," but the previous commit added a second invocation on the merged `current_request`. Rewrite the rustdoc to describe the two-phase pattern explicitly and cite both call sites. 3. **Nit coderabbit — InputFile all-None fallback intent**. Annotate the branch that rejects an `InputFile` with neither `file_data`, `file_id`, nor `file_url`. Each field is individually `Option`al per `crates/protocols/src/responses.rs`, and protocol-level validation does not currently enforce "at-least-one-source", so the arm is a defense-in-depth guard against silent-accept holes if upstream validation is relaxed. No behavior change for valid content-part inputs; only tightens the reject path against DoS-adjacent scans and documents the validator's actual contract. All 18 content_parts tests pass. `cargo clippy -p smg --all-targets -- -D warnings` clean. Refs: .claude/_audit/responses-api-gap-audit.md §Task R3 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/harmony/responses/content_parts.rs`:
- Around line 317-325: The find_base64_separator function currently scans the
first DATA_URL_HEADER_SCAN_LIMIT bytes and may match ";base64," inside early
payload bytes; restrict the search to the data URL header by first locating the
first comma (',') within the scanned slice and only running windows(..) up to
that comma index (or the scanned length if no comma), so the returned position
refers to a match before the first comma; update find_base64_separator to use
the first-comma boundary when calling windows and position while still
respecting DATA_URL_HEADER_SCAN_LIMIT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2091cbd5-0a31-4f88-b1e3-aa13c5fb2a5a
📒 Files selected for processing (1)
model_gateway/src/routers/grpc/harmony/responses/content_parts.rs
| fn find_base64_separator(input: &str) -> Option<usize> { | ||
| const NEEDLE: &[u8] = b";base64,"; | ||
| let header = input | ||
| .as_bytes() | ||
| .get(..DATA_URL_HEADER_SCAN_LIMIT.min(input.len()))?; | ||
| header | ||
| .windows(NEEDLE.len()) | ||
| .position(|window| window.eq_ignore_ascii_case(NEEDLE)) | ||
| } |
There was a problem hiding this comment.
Constrain ;base64, detection to the data-URL header (before first comma).
At Line 323, the window scan can match inside early payload bytes, not just metadata. That can misroute non-base64 data: payloads into the PDF-specific branch and violate the fallback intent documented around Lines 301-304.
Proposed fix
fn find_base64_separator(input: &str) -> Option<usize> {
const NEEDLE: &[u8] = b";base64,";
- let header = input
+ let capped = input
.as_bytes()
.get(..DATA_URL_HEADER_SCAN_LIMIT.min(input.len()))?;
- header
+ // RFC 2397: metadata header ends at the first comma.
+ // Only search for `;base64,` in that header region.
+ let header_end = capped.iter().position(|&b| b == b',').unwrap_or(capped.len());
+ let header = &capped[..header_end];
+ header
.windows(NEEDLE.len())
.position(|window| window.eq_ignore_ascii_case(NEEDLE))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/routers/grpc/harmony/responses/content_parts.rs` around
lines 317 - 325, The find_base64_separator function currently scans the first
DATA_URL_HEADER_SCAN_LIMIT bytes and may match ";base64," inside early payload
bytes; restrict the search to the data URL header by first locating the first
comma (',') within the scanned slice and only running windows(..) up to that
comma index (or the scanned length if no comma), so the returned position refers
to a match before the first comma; update find_base64_separator to use the
first-comma boundary when calling windows and position while still respecting
DATA_URL_HEADER_SCAN_LIMIT.
…odule CI's `cargo +nightly fmt -- --check` step flagged formatting differences between the local default rustfmt and the repo's nightly toolchain on the new `content_parts.rs` module. Run `cargo +nightly fmt` over the file so the whitespace/line-break choices match the project convention. No behavior or test coverage change. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e45a35730
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// megabyte `file_data` inputs that would otherwise be fully decoded | ||
| /// just to be rejected. | ||
| fn looks_like_pdf(file_data: &str) -> bool { | ||
| let payload = strip_data_url_prefix(file_data).trim_start(); |
There was a problem hiding this comment.
looks_like_pdf trims leading whitespace on the entire file_data string (strip_data_url_prefix(file_data).trim_start()) before it inspects just 8 base64 characters. On malformed input_file.file_data with a very large leading whitespace run, this turns a fast 400 rejection into an O(n) CPU walk over the full payload (which can be very large in this router), creating an avoidable request-path exhaustion vector. Bound the trim/scan to a small prefix (or skip trimming) so rejected inputs stay constant-time.
Useful? React with 👍 / 👎.
1 participant
Description
Problem
P1 added rich content parts (
ResponseContentPart::{InputImage, InputFile, Refusal}) to the Responses protocol. R1 wired them at the OpenAI-compat router via pure passthrough and R2 (parallel) wires them at the gRPC regular Responses router. Until R3, the gRPC harmony Responses router (gpt-oss path) silently dropped image and file content parts when the harmony builder rendered the prompt: theNonearms onResponseContentPart::InputImage/InputFileinharmony/builder.rsdiscarded the data, the model responded from text alone, and the caller saw no diagnostic — the worst possible failure mode for multimodal prompts.Solution
The harmony Responses pipeline is structurally different from R1 and R2: it renders prompts directly through
openai_harmonyand has no multimodal wire to the backend. The chat harmony pipeline inharmony/stages/request_building.rsexplicitly passesNonefor multimodal on every backend (sglang/vllm/trtllm), matching the fact that gpt-oss was not trained against image/file tokens at the<|message|>level. Reusinggrpc/multimodal.rsat the Responses layer without also threading bytes throughbuild_generate_request_from_responsesand the harmony prompt template would produce a prompt that advertises an image the backend cannot actually see.Rather than inventing a harmony-multimodal protocol (out of scope for a content-part transformer task), R3 takes the honest path: upgrade the pre-R3 silent drop into an explicit
400 unsupported_contentat the router entry point. The policy is centralized in a newcontent_partsmodule that both the non-streaming and streaming entry points call before any pipeline dispatch.The PDF-specific "pending R4" error message is preserved so callers can correlate the rejection with the paused R4 task instead of seeing a generic unsupported-content response.
Closes R3 on
.claude/_audit/responses-api-gap-audit.md §Task R3. Unblocks R5 (harmony row), E1 (harmony file-input row), and E2 (harmony image-input row).Changes
model_gateway/src/routers/grpc/harmony/responses/content_parts.rs(new, 370 lines with tests):Pure function
validate_harmony_responses_input(&ResponsesRequest) -> Result<(), Response>. WalksResponseInput::Items, inspectsMessageandSimpleInputMessagebodies, and rejects with a400 unsupported_content:InputImagewith any combination offile_id,image_url(absolute ordata:URL). Distinct error messages identify the specific gap.InputFilewith any combination offile_data,file_id,file_url.InputFilefile_datawhose base64-decoded prefix starts with%PDF-returns the R4-specific message so callers can correlate the paused task. Robust againstdata:application/pdf;base64,wrappers inside thefile_datafield.Refusalon any non-assistant role. Assistant-role refusals remain accepted so multi-turn replay of a prior model refusal keeps working (per the P2 §Pass 2 Drift 2 distinction).model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs: invoke the validator as the first action inserve_harmony_responses, beforeload_previous_messagesmerges persisted history. Stored assistant-refusal replays are untouched; fresh-input issues surface up-front.model_gateway/src/routers/grpc/harmony/responses/streaming.rs: mirror the validation inserve_harmony_responses_streambefore history load and SSE channel setup. Callers see a plain HTTP 400 instead of an SSE stream that immediately closes.model_gateway/src/routers/grpc/harmony/responses/mod.rs: declare the newcontent_partsmodule.model_gateway/src/routers/grpc/harmony/builder.rs: refresh the twoparse_response_item_to_harmony_message/SimpleInputMessage.Arraycomments that still referenced the open R1/R2/R3 task. TheNonearms forInputImage/InputFileremain as a defensive fallback for any future caller that bypasses the validator — documented as unreachable in practice.Why
400with a descriptive error code instead of a response built from partial context.How
&ResponsesRequest. Entry-point wiring is a one-line?(non-streaming) orif let Err(...)(streaming).file_datauses the base64 0.22 engine already in the workspace — no new dependency. The sniffer strips optionaldata:<mime>;base64,wrappers so clients that non-canonically wrap the payload still hit the PDF-specific branch.role == "assistant"through, matching the P2 §Pass 2 Drift 2 distinction between taggedMessage(user/system/developer) and assistant replay.pdf-extractdep approval. The R4 message is emitted so callers see a tracked gap.Harmony-template integration note
Image threading is not wired. gpt-oss was trained on
<|message|>text channels only; no image-token prefix or separate tensor field is read by harmony backends today (seeharmony/stages/request_building.rswhich passesNonefor multimodal on every backend). When/if multimodal harmony support lands, this validator should be relaxed and a realmultimodalparameter threaded through the harmony request_building stage on each backend.Test Plan
Unit coverage in
model_gateway/src/routers/grpc/harmony/responses/content_parts.rs(14 tests, all passing):ResponseInput::Text; pureInputTextitems;SimpleInputMessage::String; assistant-roleRefusalreplay.InputImagewith data-URL / absolute URL / file_id;InputFilewith PDF-magicfile_data(R4 message);InputFilewith JPEG-likefile_data(generic message);InputFilewithfile_url/file_id; non-assistantRefusal;SimpleInputMessage::Arraycontaining an image;data:application/pdf;base64,wrapper insidefile_data(still hits R4 branch).Workspace checks:
cargo check -p smgcargo clippy -p smg --all-targets -- -D warningscargo test -p smg --tests— 684 lib tests + other test binaries pass (0 failed)Checklist
error::bad_request("unsupported_content", ...)helper and set theX-SMG-Error-CodeheaderSummary by CodeRabbit
New Features
Tests
Documentation