feat(protocols): implement T8 custom tool + call/output items - #1301
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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:
📝 WalkthroughWalkthroughThis PR adds support for custom tool definitions and interactions to the responses protocol. New message types ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 unit tests (beta)
Comment |
github-actions
Bot
added
grpc
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean implementation of T8 custom tools. Types are well-structured, serde attributes match the spec, and all code paths (Harmony, gRPC regular, OpenAI passthrough, routing text extraction, benchmarks) properly handle the new variants. The error handling in Harmony/gRPC conversion is consistent with the existing MCP approval item pattern. Tests are comprehensive with full round-trip coverage for all variants.
0 🔴 Important · 0 🟡 Nit · 0 🟣 Pre-existing
![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: 24da7c16d5
ℹ️ 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".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/grpc/harmony/builder.rs (1)
423-439:⚠️ Potential issue | 🟠 Major
ResponseTool::Customis still silently dropped on the Harmony path.Line 435 makes custom tools count as tool-bearing input, but
ToolLike for ResponseToolstill only turnsResponseTool::Functioninto aToolDescription, so a request with onlycustomtools builds successfully while sending no tool definition to Harmony. That leaves the first turn unable to produce the expected tool call, and later turns then start failing oncecustom_tool_call*history hits Lines 726-733. Please rejectResponseTool::Customhere until Harmony can encode it explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/harmony/builder.rs` around lines 423 - 439, The code currently treats ResponseTool::Custom as a tool type but ToolLike for ResponseTool does not produce a ToolDescription for Custom, causing silent drops; update the builder to detect any ResponseTool::Custom in request.tools and return an explicit error (reject the request) instead of including "custom" in tool_types. Locate the mapping that produces tool_types (the closure iterating over request.tools) and add a branch that returns an Err/early return when encountering ResponseTool::Custom, and note that ToolLike for ResponseTool remains unchanged until Harmony gains explicit encoding support.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/protocols/src/responses.rs`:
- Around line 534-548: CustomToolCallOutputContent currently uses
ResponseContentPart for the Parts variant, which allows output_text/refusal;
replace that broad type with a new input-only enum (e.g.,
InputResponseContentPart) that only includes the three input variants
(input_text, input_image, input_file) and mirrors their serde/structure from
ResponseContentPart, then change CustomToolCallOutputContent::Parts to
Vec<InputResponseContentPart> and update any (de)serialization/validation paths
that reference CustomToolCallOutputContent or ResponseContentPart so the wire
validator rejects output/refusal types for custom_tool_call_output payloads.
In `@model_gateway/src/routers/grpc/regular/responses/conversions.rs`:
- Around line 153-160: In responses_to_chat, add an early rejection for
ResponseTool::Custom so requests that include tools with type Custom are refused
before conversion; specifically, when mapping/validating incoming tools (the
code path that later produces ResponseInputOutputItem variants like
ResponseInputOutputItem::CustomToolCall and
ResponseInputOutputItem::CustomToolCallOutput), detect ResponseTool::Custom and
return Err("Unsupported input item type" or a similar message) immediately to
prevent silent loss of custom tools later; update the conversion logic in
responses_to_chat to check for ResponseTool::Custom near the tool
extraction/mapping phase (before the branch that currently warns on
CustomToolCall) so custom tools are rejected end-to-end on the gRPC regular
route.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/harmony/builder.rs`:
- Around line 423-439: The code currently treats ResponseTool::Custom as a tool
type but ToolLike for ResponseTool does not produce a ToolDescription for
Custom, causing silent drops; update the builder to detect any
ResponseTool::Custom in request.tools and return an explicit error (reject the
request) instead of including "custom" in tool_types. Locate the mapping that
produces tool_types (the closure iterating over request.tools) and add a branch
that returns an Err/early return when encountering ResponseTool::Custom, and
note that ToolLike for ResponseTool remains unchanged until Harmony gains
explicit encoding support.
🪄 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: d1286b6a-f3c7-4262-bf67-748d365e8c44
📒 Files selected for processing (5)
crates/protocols/src/responses.rsmodel_gateway/benches/routing_allocation_bench.rsmodel_gateway/src/routers/grpc/harmony/builder.rsmodel_gateway/src/routers/grpc/regular/responses/conversions.rsmodel_gateway/src/routers/openai/responses/utils.rs
…tPart + chat-path rejection) Two defects surfaced during PR #1301 review are corrected on top of 24da7c1: 1. `CustomToolCallOutputContent::Parts` previously accepted `Vec<ResponseContentPart>`, which let spec-illegal `output_text` / `refusal` variants round-trip through `custom_tool_call_output` payloads. The spec's `output` array is restricted to `ResponseInputText | ResponseInputImage | ResponseInputFile`, so the array now holds a new `CustomToolInputContentPart` enum carrying only the three input-typed variants (`input_text`, `input_image`, `input_file`). The enum's type-level rustdoc documents the tightening and cross-references the Postel-of-liberality retained for `ResponseContentPart` at other call sites. Clippy's `enum_variant_names` lint is silenced via `#[expect(...)]` with a reason attribute because the repeated `Input` prefix intentionally mirrors the spec's wire tags. A negative `serde_json::from_value` test (`custom_tool_call_output_parts_rejects_output_typed_parts`) asserts that an `output_text` element in the array is rejected at the deserialize boundary. 2. The `chat-completions` conversion path in `model_gateway/src/routers/grpc/regular/responses/conversions.rs` silently dropped `ResponseTool::Custom(_)` entries while iterating the tools array. Silent drop risks double-charging callers and masking integration bugs. The iteration now returns `PipelineError::invalid_request(...)` on `ResponseTool::Custom(_)` with a message directing callers to the gRPC path. Addresses coderabbitai Major review comment 3121061850 (type-level restriction + negative test) and Nitpick comment 3121061854 (chat-path rejection), plus codex P2 observations covering the same two concerns. Refs: T8 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
| return Err("Unsupported tool type".to_string()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing: The Harmony pipeline has the same silent-drop gap — preparation.rs:175 calls extract_tools_from_response_tools without a preceding ResponseTool::Custom rejection. Custom tool items are rejected at builder.rs:726-732, but a request carrying tools: [{type: "custom", …}] with no custom items would silently lose the tool definition. Not introduced by this PR, but worth a follow-up since the pattern is now established here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfb03552f7
ℹ️ 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".
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/regular/responses/conversions.rs`:
- Around line 173-191: In responses_to_chat, besides checking req.tools for
ResponseTool::Custom, also inspect the request's tool_choice field (e.g.,
req.tool_choice or equivalent enum) and return an Err when its variant/type is
"custom"; specifically, detect tool_choice == Custom (or the string "custom")
and fail fast with the same error message used for ResponseTool::Custom so
requests with a custom tool_choice are rejected at the same conversion boundary
rather than being silently downgraded to auto.
🪄 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: 753116b4-a04f-4fae-8f6b-36eb8b5adb0a
📒 Files selected for processing (2)
crates/protocols/src/responses.rsmodel_gateway/src/routers/grpc/regular/responses/conversions.rs
…to harmony + tool_choice)
What changed
- model_gateway/src/routers/grpc/regular/responses/conversions.rs:
- Import `ResponsesToolChoice`.
- After the existing `ResponseTool::Custom` tool-array rejection in
`responses_to_chat`, also reject `req.tool_choice` when it is
`ResponsesToolChoice::Custom { .. }`. Returns the same-shape
`Err("Unsupported tool choice")` so the chat path no longer silently
downgrades a custom tool_choice to `auto` via `to_chat_tool_choice`.
- New test `test_custom_tool_choice_rejected_on_chat_path` locks in
the behaviour for requests carrying `tool_choice: {"type":"custom"}`
but no custom tool in `tools`.
- model_gateway/src/routers/grpc/harmony/stages/preparation.rs:
- Import `ResponseTool`, `ResponsesToolChoice`, and `warn`.
- At the top of `prepare_responses`, before
`extract_tools_from_response_tools` runs, reject both
`ResponseTool::Custom` entries in `request.tools` and
`ResponsesToolChoice::Custom { .. }` in `request.tool_choice` with
`error::bad_request("unsupported_tool_type" / "unsupported_tool_choice",
...)`. This closes the silent-drop gap the Harmony path previously
had: custom tool definitions were accepted and then stripped by
`extract_tools_from_response_tools` / `to_chat_tool_choice` without
reaching the model.
- model_gateway/src/routers/grpc/harmony/builder.rs:
- Convert the `tool_types` match in `construct_input_messages_with_harmony`
from infallible `.map(...).collect()` into a fallible
`.map(|tool| match tool { ... })` that returns
`Err("Unsupported tool type")` for `ResponseTool::Custom` and is
collected as `Result<Vec<_>, _>`. Propagated via `?`, this surfaces
as a string error up through `build_from_responses` →
`prepare_responses` → `error::bad_request`.
- Adds defense-in-depth at the builder boundary: even if a future
caller invokes `build_from_responses` without passing through
`prepare_responses`, custom tools are not silently dropped.
- New test `test_custom_tool_rejected_in_harmony_build` exercises the
builder boundary directly.
Why
- Three convergent round-2 review comments flagged the same underlying
gap: Custom-variant rejection added in PR #1301 only covered
`ResponseTool::Custom` on the chat path, leaving `tool_choice::Custom`
and the entire Harmony path as silent-downgrade corridors. A request
with `tools: [{"type":"custom", ...}]` or
`tool_choice: {"type":"custom", ...}` on the Harmony path was
accepted, had its custom definition stripped, and ran as if the
caller had asked for `auto` — the model never saw the custom tool
and could not emit the expected `custom_tool_call` output. Postel's
law cuts the other way here: the gateway should fail loudly on
unrepresentable inputs, not silently change caller semantics.
How
- Mirrored the existing chat-path rejection pattern (warn! + Err
return of "Unsupported tool type") in three places so rejection is
complete across:
1. chat path tools (pre-existing, unchanged),
2. chat path tool_choice (new, conversions.rs),
3. Harmony path tools + tool_choice (new, preparation.rs
single-site guard for both new and continuing conversations),
4. Harmony builder boundary (new, builder.rs fallible collect as
defense-in-depth for direct callers).
- Uses the same error shape per site: chat path keeps the plain
`String` return type already in `responses_to_chat`; Harmony
preparation path uses `error::bad_request` (the existing error
helper for that stage); builder path returns `Err(String)` that the
preparation stage translates through its existing `map_err` into
`bad_request`.
- Two regression tests (one per path) prevent reintroduction of the
silent-drop behaviour.
Verification
- cargo fmt --all --check (clean, only unstable-feature warnings)
- cargo check -p smg (clean)
- cargo clippy -p smg --lib --tests -- -D warnings (clean)
- cargo test -p smg --lib -- routers::grpc (39 passed / 0 failed,
both new tests included)
- cargo test -p smg --lib -- responses (19 passed / 0 failed)
- cargo test -p smg --lib -- harmony (2 passed / 0 failed)
- codespell on the three modified files (clean)
Addresses
- coderabbitai review comment 3121598363 (Major, chat path
tool_choice silent downgrade)
- codex review comment 3121584043 (P1, Harmony conversion accepts
custom tools but cannot emit them)
- claude review comment 3121583469 (Pre-existing, Harmony preparation
path silent-drop gap)
Refs: T8
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/builder.rs`:
- Around line 423-451: The scan that rejects ResponseTool::Custom must be moved
earlier so it runs before any early-return branch that checks
previous_response_id; update construct_input_messages_with_harmony to validate
request.tools for any ResponseTool::Custom (matching the current mapping that
returns Err("Unsupported tool type")) before the code path that splits or
delegates to build_from_responses when previous_response_id is set, ensuring
continuations can't silently drop custom tools, and add a regression test that
constructs a request with previous_response_id set and a ResponseTool::Custom to
verify the function returns the same unsupported-tool error.
🪄 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: 9d7482d2-3b3e-4103-acd2-8cf472f252c6
📒 Files selected for processing (3)
model_gateway/src/routers/grpc/harmony/builder.rsmodel_gateway/src/routers/grpc/harmony/stages/preparation.rsmodel_gateway/src/routers/grpc/regular/responses/conversions.rs
…previous_response_id split)
What changed:
- model_gateway/src/routers/grpc/harmony/builder.rs
- Move the `ResponseTool::Custom` rejection scan in
`construct_input_messages_with_harmony` from inside the
`previous_response_id.is_none()` arm to the top of the function,
before the new-vs-continuing branch split. The `tool_types` vec is
still computed unconditionally and reused by the new-conversation
branch for `has_custom_tools`; the continuing-conversation branch
now benefits from the same validation pass instead of skipping it.
- Add regression test
`test_custom_tool_rejected_in_harmony_build_with_previous_response_id`
covering the exact case flagged by the reviewer: a request with
`previous_response_id = Some(..)` and a `ResponseTool::Custom`
must still surface the `Unsupported tool type` error.
Why:
- coderabbitai Major (PR #1301, harmony/builder.rs:451): the
`ResponseTool::Custom` validation only ran for new conversations.
When `previous_response_id` was set, `build_from_responses()`
skipped the rejection scan, ignored `request.tools`, and still
succeeded — continuations could silently drop custom tools, leaving
later turns unable to produce the expected tool call and eventually
failing once `custom_tool_call*` history items hit the Harmony
item-level rejection downstream.
How:
- The scan runs first and returns `Err("Unsupported tool type")` the
moment any `ResponseTool::Custom` appears in `request.tools`,
regardless of the continuation state. The new-conversation branch
still uses the computed `tool_types` for `has_custom_tools`, so
there is no behavioral change for supported tool kinds
(`function`, `web_search_preview`, `code_interpreter`, `mcp`,
`file_search`).
- Narrow gate: `cargo check -p smg --tests` passes, and both
`test_custom_tool_rejected_in_harmony_build` and the new
`test_custom_tool_rejected_in_harmony_build_with_previous_response_id`
pass (2/2 ok).
Refs: T8
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
be01408 to
9e697cc
Compare
|
Branch rewritten to restore schema-only scope discipline. What was dropped (scope bleed / defensive elaboration):
What was kept (forced by the compiler; minimum arms matching existing arm shape):
Final diff: 548 lines total (432 protocol / 21 router / rest is test code inside the protocol tests mod). Protocol additions cover Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e697cc704
ℹ️ 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".
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
9e697cc to
fa3eef3
Compare
mergify
Bot
removed
the
needs-rebase
|
|
||
| /// Mask reference for image-generation `edit` calls. Spec: `{ file_id?, image_url? }`. | ||
| /// Reuses the same upload conventions as `InputImage`. | ||
| /// Reuses the same upload conventions as P1 `InputImage`. |
There was a problem hiding this comment.
🟡 Nit: Spurious "P1" prefix — looks like an internal task label that leaked into the doc comment.
| /// Reuses the same upload conventions as P1 `InputImage`. | |
| /// Reuses the same upload conventions as `InputImage`. |
There was a problem hiding this comment.
Out of scope for this PR — the P1 InputImage doc comment was introduced by T4 (#1303) and is unrelated to T8's custom tool additions. Keeping T8's diff schema-only per audit playbook §7; happy to clean these labels up in a follow-up docs pass across all audit tasks.
| /// to a single-value tag enum so payloads with an unknown `type` | ||
| /// (e.g. `"input_file"`, `"totally_made_up"`) do not silently land | ||
| /// in this untagged catch-all variant. | ||
| /// in this untagged catch-all variant — P5 fail-fast contract. |
There was a problem hiding this comment.
🟡 Nit: "P5 fail-fast contract" reads like an internal task/priority label. The comment already explains the mechanism clearly without it — the tag adds no value to a future reader.
| /// in this untagged catch-all variant — P5 fail-fast contract. | |
| /// in this untagged catch-all variant. |
Same on line 1027.
There was a problem hiding this comment.
Out of scope for this PR — the P5 fail-fast contract label was introduced by P5 (c8996dc) and predates T8. Keeping T8's diff schema-only per audit playbook §7; suitable for a follow-up docs pass.
| ResponseInputOutputItem::CustomToolCall { input, .. } => { | ||
| if input.is_empty() { | ||
| let mut e = ValidationError::new("custom_tool_call_input_empty"); | ||
| e.message = Some("Custom tool call input cannot be empty".into()); | ||
| return Err(e); | ||
| } |
There was a problem hiding this comment.
🟡 Nit: CustomToolCall.input is the model's free-form payload echoed back by the client in a multi-turn conversation. Rejecting "" here means that if the model ever produces a custom_tool_call with empty input (plausible for a parameterless tool using unconstrained format: text), the client's follow-up request will fail validation on the re-submitted item.
The other model-generated call variants — FunctionToolCall (line 2227) and ImageGenerationCall (line 2230) — have no analogous content validation, so this is asymmetric. (FunctionToolCall.arguments is always JSON so "" is naturally impossible, but the validation function doesn't enforce that either.)
The output-side validation (CustomToolCallOutput below, FunctionCallOutput above) is fine — those are client-authored and the "non-empty output" contract is consistent.
Consider dropping this check to match the existing call-side pattern, or at minimum adding a test that documents the intent (the test file covers serde round-trips but not validation).
There was a problem hiding this comment.
Good catch — dropped the asymmetric check in 7030bfe. CustomToolCall.input is now unvalidated at this layer, matching the existing FunctionToolCall arm. Output-side rejection on CustomToolCallOutput stays (client-authored, consistent with FunctionCallOutput).
Remove the is_empty() rejection on `CustomToolCall.input` in `validate_input_item`. The `input` field is the model's free-form payload that clients must echo back unchanged on multi-turn replay — if the model emits an empty-input `custom_tool_call` (plausible for a parameterless tool using `format: text`), the follow-up request would fail validation on the re-submitted item. FunctionToolCall (the closest call-side analogue) has no equivalent content validation, so this check was also asymmetric. Keep CustomToolCallOutput's non-empty check — the output side is client-authored and the non-empty-output contract is consistent with FunctionCallOutput. Refs: PR #1301 review Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Update the doc comments for `ResponseInputOutputItem::CustomToolCall` and `::CustomToolCallOutput` so the spec-shape string matches the Rust types: `id?` / `namespace?` instead of bare `id, namespace`. Also add a sentence explaining that these are modelled as `Option<String>` for newly-minted client-side calls (they are populated on round-tripped items from a previous response). No wire-shape or type change — the fields already serialize with `skip_serializing_if = "Option::is_none"`. Refs: PR #1301 review Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
Remove the is_empty() rejection on `CustomToolCall.input` in `validate_input_item`. The `input` field is the model's free-form payload that clients must echo back unchanged on multi-turn replay — if the model emits an empty-input `custom_tool_call` (plausible for a parameterless tool using `format: text`), the follow-up request would fail validation on the re-submitted item. FunctionToolCall (the closest call-side analogue) has no equivalent content validation, so this check was also asymmetric. Keep CustomToolCallOutput's non-empty check — the output side is client-authored and the non-empty-output contract is consistent with FunctionCallOutput. Refs: PR #1301 review Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Update the doc comments for `ResponseInputOutputItem::CustomToolCall` and `::CustomToolCallOutput` so the spec-shape string matches the Rust types: `id?` / `namespace?` instead of bare `id, namespace`. Also add a sentence explaining that these are modelled as `Option<String>` for newly-minted client-side calls (they are populated on round-tripped items from a previous response). No wire-shape or type change — the fields already serialize with `skip_serializing_if = "Option::is_none"`. Refs: PR #1301 review Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
930f992 to
a19a329
Compare
mergify
Bot
removed
the
needs-rebase
| }, | ||
| other => panic!("expected CustomToolCallOutput, got {other:?}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Nit: The new tests thoroughly cover serde round-trips and the output_text/refusal rejection, but the validate_input_item paths for CustomToolCallOutput are untested. Specifically, the empty-string ("output": "") and empty-array ("output": []) rejection branches at responses.rs:2241-2253 have no coverage — the validation could regress silently if someone refactors the match arms or reorders the guard clauses.
Consider adding a test that constructs a ResponsesRequest with an empty CustomToolCallOutput and asserts req.validate().is_err().
There was a problem hiding this comment.
Added test_custom_tool_call_output_validation_rejects_empty_text_and_parts in a13b9a1 that covers both the empty-string and empty-parts rejection branches plus a non-empty sanity case. The test pairs each output item with a user message so the cross-parameter "must contain at least one message" gate doesn't short-circuit the branch we want to cover.
Add validation coverage for the two `validate_input_item` branches on `CustomToolCallOutput`: - `CustomToolCallOutputContent::Text(s) if s.is_empty()` → reject - `CustomToolCallOutputContent::Parts(parts) if parts.is_empty()` → reject Plus a sanity case (non-empty text) that must still validate. All three payloads pair the tool output with a user message because the cross-parameter validator requires at least one `Message` / `SimpleInputMessage` in the input list. Refs: PR #1301 review Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
Schema-only rewrite of the T8 branch. This is a clean re-application
against current origin/main (which advanced through T3/T4/P5/P6) and
drops the defensive router guardrails that the earlier round-2/round-3
review iterations had layered on top. Those guardrails turned schema
tasks into behavior elaboration and violated scope discipline.
What changed (protocols only, plus forced-cascade match arms):
- crates/protocols/src/responses.rs
- Add ResponseTool::Custom(CustomTool) variant with #[serde(rename = "custom")].
- Add CustomTool struct (name, description?, defer_loading?, format?).
- Add CustomToolInputFormat enum (Text | Grammar) tagged by "type".
- Add CustomToolGrammar { definition, syntax } and CustomToolGrammarSyntax
enum (Lark | Regex).
- Add CustomToolInputContentPart (input-only content part: input_text,
input_image, input_file) — restricts the custom_tool_call_output array
form to spec-legal variants, rejecting output_text/refusal at the type
boundary.
- Add CustomToolCallOutputContent (untagged Text(String) | Parts(Vec<...>)).
- Add ResponseInputOutputItem::CustomToolCall { call_id, input, name, id?,
namespace? } with #[serde(rename = "custom_tool_call")].
- Add ResponseInputOutputItem::CustomToolCallOutput { call_id, output, id? }
with #[serde(rename = "custom_tool_call_output")]; no status field per spec.
- Extend validate_input_item + extract_text_for_routing with match arms
for the two new input items.
- Six serde round-trip tests covering Text format, both grammar syntaxes,
custom_tool_call, custom_tool_call_output (string), custom_tool_call_output
(array), and a negative test for output_text/refusal rejection.
Forced-cascade match arms (compiler-required, one line or Ok-shaped each):
- model_gateway/src/routers/grpc/harmony/builder.rs
- tool_types match: ResponseTool::Custom(_) => "custom" (matches &str arms).
- parse_response_item_to_harmony_message match: CustomToolCall/Output arms
return Err("Unsupported input item type") + warn!(), matching the
existing McpApprovalRequest/ImageGenerationCall arm shape.
- model_gateway/src/routers/grpc/regular/responses/conversions.rs
- responses_to_chat item match: CustomToolCall/Output arms return
Err("Unsupported input item type") + warn!(), matching the existing
McpApprovalResponse/ImageGenerationCall arm shape.
- model_gateway/src/routers/openai/responses/utils.rs
- response_tool_to_value: ResponseTool::Custom(_) => serde_json::to_value(tool).ok()
matching the existing FileSearch/ImageGeneration arm shape.
- model_gateway/benches/routing_allocation_bench.rs
- extract_text_for_routing_old filter_map: CustomToolCall/Output => None
matching the existing McpApprovalRequest/ImageGenerationCall arms.
Explicitly NOT included (dropped as scope bleed during rewrite):
- Defensive pre-check blocks rejecting ResponseTool::Custom in
preparation.rs and conversions.rs (not compiler-required).
- Defensive rejection of ResponsesToolChoice::Custom (not compiler-required;
P7 landed that variant in #1276 without router changes).
- Refactor of the tool_types match from infallible .map(...) into
fallible .map(|t| match ...).collect::<Result<_, _>>() (elaboration).
- Router-layer regression tests for the dropped rejection blocks.
Why:
Closes the T8 gap in the Responses API audit. Spec
(openai-responses-api-spec.md L471-474 for the tool and L268-273 for the
input items) defines the `custom` tool + `custom_tool_call` /
`custom_tool_call_output` as first-class wire shapes.
How:
Variant placement follows the established single-rename pattern used by
FileSearch (T1) / ImageGeneration (T4) / WebSearch (T3). Grammar
sub-union is internally tagged by type. CustomToolCallOutput.output is
untagged because the spec accepts either a raw string or an array of
input content parts. Every non-exhaustive match on
ResponseInputOutputItem or ResponseTool was audited and extended (four
call sites + the bench harness) — no silent default arms added, no
behavior-level guards added.
Drift: audit "Desired" listed CustomToolCallOutput with an optional
`status` field; spec has no such field. Implementation follows the spec.
Refs: audit task T8
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Remove the is_empty() rejection on `CustomToolCall.input` in `validate_input_item`. The `input` field is the model's free-form payload that clients must echo back unchanged on multi-turn replay — if the model emits an empty-input `custom_tool_call` (plausible for a parameterless tool using `format: text`), the follow-up request would fail validation on the re-submitted item. FunctionToolCall (the closest call-side analogue) has no equivalent content validation, so this check was also asymmetric. Keep CustomToolCallOutput's non-empty check — the output side is client-authored and the non-empty-output contract is consistent with FunctionCallOutput. Refs: PR #1301 review Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Update the doc comments for `ResponseInputOutputItem::CustomToolCall` and `::CustomToolCallOutput` so the spec-shape string matches the Rust types: `id?` / `namespace?` instead of bare `id, namespace`. Also add a sentence explaining that these are modelled as `Option<String>` for newly-minted client-side calls (they are populated on round-tripped items from a previous response). No wire-shape or type change — the fields already serialize with `skip_serializing_if = "Option::is_none"`. Refs: PR #1301 review Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Add validation coverage for the two `validate_input_item` branches on `CustomToolCallOutput`: - `CustomToolCallOutputContent::Text(s) if s.is_empty()` → reject - `CustomToolCallOutputContent::Parts(parts) if parts.is_empty()` → reject Plus a sanity case (non-empty text) that must still validate. All three payloads pair the tool output with a user message because the cross-parameter validator requires at least one `Message` / `SimpleInputMessage` in the input list. Refs: PR #1301 review Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
a13b9a1 to
ccfe476
Compare
mergify
Bot
removed
the
needs-rebase
1 participant
Summary
Implements audit task T8:
customtool +custom_tool_call/custom_tool_call_outputinput items.What changed
crates/protocols/src/responses.rsResponseTool::Custom(CustomTool)variant (#[serde(rename = "custom")]).CustomTool { name, description?, defer_loading?, format? }withdeny_unknown_fields.CustomToolInputFormatenum (Text|Grammar) internally tagged bytype.CustomToolGrammar { definition, syntax }andCustomToolGrammarSyntaxenum (Lark|Regex).ResponseInputOutputItem::CustomToolCall { call_id, input, name, id?, namespace? }.ResponseInputOutputItem::CustomToolCallOutput { call_id, output, id? }— nostatusfield (see drift note).CustomToolCallOutputContentuntagged enum —Text(String)|Parts(Vec<ResponseContentPart>).extract_text_for_routingandvalidate_input_itemmatch arms.model_gateway/src/routers/grpc/harmony/builder.rs— added the two new input-item arms inparse_response_item_to_harmony_messageand theResponseTool::Customarm in the tool-type labeler.model_gateway/src/routers/grpc/regular/responses/conversions.rs— added the two new input-item arms inresponses_to_chat.model_gateway/src/routers/openai/responses/utils.rs— added theResponseTool::Customarm inresponse_tool_to_value.model_gateway/benches/routing_allocation_bench.rs— added the two new input-item arms to the exhaustive routing-text extraction.Why
Spec
openai-responses-api-spec.mdL471-474 defines thecustomtool:and L268-273 defines the paired input items:
Without these types smg cannot round-trip user-defined tool flows; previous turns emitting
custom_tool_callwould be rejected byResponseInputOutputItemdeserialisation.ToolChoice::Customalready landed in #1276 (P7), so no new tool-choice variant is required.Verification
cargo +nightly fmt --all -- --check— clean (silent)cargo clippy -p openai-protocol --all-targets --all-features -- -D warnings— cleancargo clippy -p smg --all-targets --all-features -- -D warnings— cleancargo test -p openai-protocol --lib responses::tests— 35/35 passing, incl. 5 new T8 testspre-commit run codespell— Passedconversions.rsarm produceserror[E0004]: non-exhaustive patterns: CustomToolCall, CustomToolCallOutput not coveredcodex: unavailable(§8 solo-fallback — Tech Lead APPROVE after hand round-trip suffices)matchonResponseTool/ResponseInputOutputItemnow covers the new variantsBlast radius
5 files changed, +333 / −1:
No
Cargo.toml/Cargo.lock/bindings//.github//.cargo/changes.Out of scope
namespacetool grouping — tracked as T9 (blocked on T8; this PR unblocks it).Unsupported input item type); this PR only lands wire types.CustomToolCallOutput { ..., status? }but spec L268 has nostatusfield. Implemented per spec; drift logged in.claude/_audit/responses-api-gap-audit.md→ Drift Log.#[serde(deny_unknown_fields)]is intentionally absent on theCustomToolCallOutputstruct variant to match sibling variants (FunctionCallOutput, etc.); hardening silent-field drops is tracked separately as P5.Refs: T8
Summary by CodeRabbit