feat(protocols): implement T2 computer / computer_use_preview tools - #1305
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
Bot
added
grpc
|
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:
📝 WalkthroughWalkthroughAdds computer-use tools and input/output variants to Responses protocol, updates routing/serialization/validation and tests, excludes computer-call items from routing text, marks computer-call outputs visible in MCP, treats computer-call items as unsupported in some gateway conversions, and updates codespell ignore list. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCP Session
participant Protocol as Responses Protocol
participant Gateway as Model Gateway
participant Service as OpenAI/gRPC
Client->>MCP: submit request with computer action
MCP->>Protocol: create/emit `ComputerCall`
Protocol->>Protocol: serialize/deserialize Computer tool/call/output
Protocol-->>MCP: return `ComputerCallOutput`
MCP->>MCP: is_client_visible_output_item -> true
MCP->>Gateway: forward visible computer item
Gateway->>Gateway: map/serialize `ResponseTool::Computer` or flag unsupported
Gateway->>Service: send tool payload or return unsupported error
Service-->>Gateway: conversion result or error
Gateway-->>Client: final response or error
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 unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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-442:⚠️ Potential issue | 🟠 MajorTreat
computer*tools as built-ins inhas_custom_tools.These new variants are added to
tool_types, butBUILTIN_TOOLSstill omits"computer"and"computer_use_preview". A request that only exposes computer tools is therefore classified as having custom tools, which changes the Harmony prompt shape by leaving the commentary channel enabled and injecting an empty developer message even though no custom tool descriptions exist. Please add both names to the built-in set (or derive this fromResponseTooldirectly).🔧 Proposed fix
const BUILTIN_TOOLS: &[&str] = &[ "web_search_preview", "code_interpreter", "container", "file_search", + "computer", + "computer_use_preview", ];🤖 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 - 442, The tool detection logic treats newly added ResponseTool variants for computers as custom; update the built-in list used by has_custom_tools to include "computer" and "computer_use_preview" (or change has_custom_tools to derive built-ins from ResponseTool enum) so requests that only expose Computer/ComputerUsePreview are not treated as custom; locate the code that defines BUILTIN_TOOLS and the has_custom_tools call (referenced here via tool_types and ResponseTool::Computer / ResponseTool::ComputerUsePreview) and add those two string names to the built-in set or implement a derived set based on ResponseTool variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/routers/grpc/harmony/builder.rs`:
- Around line 423-442: The tool detection logic treats newly added ResponseTool
variants for computers as custom; update the built-in list used by
has_custom_tools to include "computer" and "computer_use_preview" (or change
has_custom_tools to derive built-ins from ResponseTool enum) so requests that
only expose Computer/ComputerUsePreview are not treated as custom; locate the
code that defines BUILTIN_TOOLS and the has_custom_tools call (referenced here
via tool_types and ResponseTool::Computer / ResponseTool::ComputerUsePreview)
and add those two string names to the built-in set or implement a derived set
based on ResponseTool variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e27e48a0-401c-474e-87f3-56d57b40c66b
📒 Files selected for processing (7)
.pre-commit-config.yamlcrates/mcp/src/core/session.rscrates/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
![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: 02f049f2cd
ℹ️ 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".
| keys: Option<Vec<String>>, | ||
| }, | ||
| /// `{ type: "double_click", keys, x, y }`. | ||
| DoubleClick { keys: Vec<String>, x: i32, y: i32 }, |
There was a problem hiding this comment.
DoubleClick currently models keys as Vec<String>, which makes deserialization fail when upstream sends "keys": null (a valid shape in the OpenAI-generated Responses schema, where the field is required but nullable). In that case, otherwise valid computer_call payloads are rejected at the protocol boundary instead of round-tripping.
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 `@crates/protocols/src/responses.rs`:
- Around line 583-586: The doc comment for the MouseButton enum incorrectly
states it is used by `Scroll` actions; update the comment to only reference
actions that actually accept a button (e.g., `Click`) and remove or reword any
mention of `Scroll` to avoid confusion—edit the docstring above the
`MouseButton` type (the derive block for MouseButton) to say something like
"Mouse button used by `Click` actions" and ensure it still documents the allowed
values per the spec.
- Around line 840-875: Duplicate field shapes in the enum variants ComputerCall
and ComputerCallOutput (as used in ResponseInputOutputItem and
ResponseOutputItem) risk enum drift; extract their common payload fields into
new shared structs (e.g., ComputerCallPayload and ComputerCallOutputPayload) and
replace the duplicate fields with a single flattened field using
#[serde(flatten)] in the ComputerCall and ComputerCallOutput variants (and
update ResponseInputOutputItem/ResponseOutputItem usages to use the new structs)
so serde preserves the same JSON shape while centralizing shared fields like id,
call_id, action/actions, status, pending_safety_checks, output, and
acknowledged_safety_checks.
🪄 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: 42501c55-8f59-438d-b6ec-a54c1ce43410
📒 Files selected for processing (1)
crates/protocols/src/responses.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1248a858f4
ℹ️ 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".
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| status: Option<ComputerCallStatus>, |
There was a problem hiding this comment.
failed status on computer_call_output items
ResponseOutputItem::ComputerCallOutput models status as Option<ComputerCallStatus>, but ComputerCallStatus only allows in_progress|completed|incomplete. In provider responses, computer_call_output can be emitted with status: "failed"; those payloads will fail deserialization and get dropped by history replay paths (e.g., previous-response chain loading), which loses tool state exactly when a computer step errors and a retry/handoff is needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging — verified against the OpenAI Python SDK and pushing back on this one.
types/responses/response_computer_tool_call_output_item.py:34-40 (openai==2.8.1, Stainless-generated from OpenAPI):
status: Optional[Literal["in_progress", "completed", "incomplete"]] = None
"""The status of the message input.
One of `in_progress`, `completed`, or `incomplete`. Populated when input items
are returned via API.
"""The spec constrains ResponseComputerToolCallOutputItem.status to exactly three literals; failed is not a valid variant. The current enum (in_progress/completed/incomplete) matches the SDK contract, so adding Failed would diverge from the wire format. Keeping the existing three-variant enum. No change in 631b4be.
…always-serialize + MouseButton docstring) Round-2 bot review on PR #1305 raised four issues; three actionable fixes and one dismissed with spec justification. Changes - `ResponseInputOutputItem::ComputerCall.pending_safety_checks` and `ResponseOutputItem::ComputerCall.pending_safety_checks`: drop the `skip_serializing_if = "Vec::is_empty"` serde attribute so an empty array serializes as `[]` on the wire rather than being omitted. - `MouseButton` docstring: remove the misleading "Click/Scroll" attribution and clarify that only the `Click` action carries a `button` field; `Scroll` uses `scroll_x`/`scroll_y` offsets and has no button parameter. - Tests: * extend `test_computer_call_output_item_round_trip` to include the now-mandatory empty `pending_safety_checks: []` in its round-trip fixture. * add `test_computer_call_pending_safety_checks_always_serialized` to guard both `ResponseInputOutputItem::ComputerCall` and `ResponseOutputItem::ComputerCall` against regression — confirms the field is emitted as `[]` even when the source Vec is empty. Why - OpenAI Python SDK (openai==2.8.1, `types/responses/response_computer_tool_call.py:203`) declares `pending_safety_checks: List[PendingSafetyCheck]` as a non-`Optional` field. Empty array is semantically distinct from field omission and SDK consumers expect the key to always be present. - `Scroll` action (L651-659) has no `button` field — only `scroll_x`/`scroll_y`/`x`/`y`/`keys`. The old docstring claimed `button` applied to both `Click` and `Scroll`, which is incorrect. How - Replaced `#[serde(default, skip_serializing_if = "Vec::is_empty")]` with `#[serde(default)]` at both call sites to preserve deserialization tolerance (absent field → empty Vec) while forcing serialization to always emit the field. - Inline doc-comments on each field cite the SDK source line for future auditors. - Updated existing round-trip test payload to include the explicit empty array; new dedicated test deserializes payloads without the field, re-serializes, and asserts `pending_safety_checks == []` to pin the invariant. Dismissed - codex P1 (3121550562, responses.rs:1131) requested adding `Failed` to `ComputerCallStatus`. The OpenAI Python SDK constrains `ResponseComputerToolCallOutputItem.status` to `Literal["in_progress", "completed", "incomplete"]` (`response_computer_tool_call_output_item.py:34-40`), so `Failed` is not a valid variant. Keeping current three-variant enum matches spec. - coderabbitai trivial (3121540405, responses.rs:875) proposed extracting shared `ComputerCall`/`ComputerCallOutput` payload structs. Variant-scoped inline structs are intentional: each enum site uses its own `#[serde(rename = ...)]` discriminator and opts into slightly different serde attributes (e.g. the output-side variant is currently `#[serde(untagged)]`-adjacent to other patterns). Extracting would force these variations into a single struct and obscure the per-variant schema. Documentation comments already cite the spec, so drift risk is addressed by the §7-style cross-references. Verification - `cargo fmt --all -- --check` (clean) - `cargo clippy -p openai-protocol --all-targets -- -D warnings` (0 warnings) - `cargo test -p openai-protocol` (89 + 8 + 18 + 1 doc-tests = all green, including new `test_computer_call_pending_safety_checks_always_serialized`) Refs: T2 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 `@crates/protocols/src/responses.rs`:
- Around line 2058-2059: The validator validate_responses_cross_parameters()
still enforces a Message/SimpleInputMessage even when the continuation is a
tool-only follow-up; update that validator to recognize
ResponseInputOutputItem::ComputerCall and
ResponseInputOutputItem::ComputerCallOutput as valid continuation items that do
not require input_missing_user_message when a previous_response_id is provided
(e.g., allow previous_response_id + [{ "type": "computer_call_output", ... }] as
valid). Specifically, adjust the branch that checks for presence of user
messages to skip that check if all continuation items are
ComputerCall/ComputerCallOutput, and ensure the error code
input_missing_user_message is not emitted in that case while preserving existing
checks for other item types.
🪄 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: 395b7a00-f875-441c-965a-e4935bfc5a20
📒 Files selected for processing (1)
crates/protocols/src/responses.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 631b4bed27
ℹ️ 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".
…w-ups + computer tools BUILTIN_TOOLS) Two bot-review comments on PR #1305 after round-2: 1. coderabbitai Critical (responses.rs:2059) — the cross-parameter validator at `validate_responses_cross_parameters()` (crates/protocols/src/responses.rs:1968) still required at least one `Message` or `SimpleInputMessage` in the input array. That breaks the computer-use multi-turn flow the T2 variants enabled: a continuation turn sending just `[ComputerCall {...}, ComputerCallOutput {...}]` alongside a `previous_response_id` (or `conversation`) was rejected with `input_missing_user_message`, leaving the new variants unreachable after turn 1. Relax the validator so: - If `previous_response_id` is set OR `conversation` is set, skip the "at least one message" gate entirely (the prior turn already established the user message; the follow-up is a continuation). - Otherwise, accept any of the user-turn or tool-item variants (`Message`, `SimpleInputMessage`, `FunctionToolCall`, `FunctionCallOutput`, `McpApprovalRequest`, `McpApprovalResponse`, `ComputerCall`, `ComputerCallOutput`). A reasoning-only payload without continuation still fails fast. Added three regression tests in `crates/protocols/src/responses.rs`: tool-only follow-up with `previous_response_id` (accepted), tool-only follow-up with `conversation` id (accepted), and reasoning-only new turn (rejected with code `input_missing_user_message`). Tests discover the error code through `ValidationErrors::errors()` because the `validator` crate surfaces schema-level errors under the synthetic `__all__` field. 2. codex P2 (harmony/builder.rs:436) — same pattern as the T1 / T3 / T4 fixes: `extract_tool_types_from_response_tools` (L435-436) emits `"computer"` and `"computer_use_preview"` into `tool_types`, but those strings were missing from the `BUILTIN_TOOLS` slice, so `has_custom_tools()` returned `true` for computer-only requests and the harmony builder would pin them into the custom-tool path (adds an unnecessary developer message, skews system prompt). Add both strings to `BUILTIN_TOOLS` (L63-68) and add `ResponseTool::Computer | ResponseTool::ComputerUsePreview(_)` to the `matches!` arm in `ToolLike::is_builtin()` for `ResponseTool` (L108-115) so the built-in contract stays in sync with the extraction map. Verification: CARGO_TARGET_DIR=/tmp/t2-r3-target cargo check -p smg --lib OK CARGO_TARGET_DIR=/tmp/t2-r3-target cargo test -p openai-protocol --lib 92 passed CARGO_TARGET_DIR=/tmp/t2-r3-target cargo test -p smg --lib 566 passed (4 ignored) No source-code behaviour changes beyond the two reviewed points; no doc changes; no touch to files outside the T2 charter. Refs: T2 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: cf7c0eab12
ℹ️ 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: 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 `@crates/protocols/src/responses.rs`:
- Around line 685-691: The struct ComputerSafetyCheck currently requires code
and message but the API marks them optional; change the types of
ComputerSafetyCheck.code and ComputerSafetyCheck.message from String to
Option<String> (e.g., Option<String>) so serde will accept missing fields, keep
the existing derives (Deserialize/Serialize/schemars::JsonSchema), and update
any usage sites that access ComputerSafetyCheck.code or .message to handle None
(unwraps/assumptions should be guarded or fall back to a default).
- Around line 1968-2005: The validator must reject requests that include
computer-use items when truncation is not set to "auto": update the validation
after the items check to scan items for ResponseInputOutputItem::ComputerCall
and ResponseInputOutputItem::ComputerCallOutput and, if any are present, ensure
request.truncation is Some("auto") (or the Truncation::Auto enum variant used in
this crate); if not, return a ValidationError (e.g.,
"truncation_must_be_auto_for_computer_use_preview") with a message telling the
caller to set truncation to "auto". Use the existing request,
request.truncation, and the ResponseInputOutputItem::ComputerCall /
::ComputerCallOutput symbols to locate where to add this check.
🪄 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: c922b460-c21d-4592-b412-89d34db5674b
📒 Files selected for processing (2)
crates/protocols/src/responses.rsmodel_gateway/src/routers/grpc/harmony/builder.rs
| // 5. Validate input items structure. | ||
| // | ||
| // Continuation turns (driven by `previous_response_id` or `conversation`) | ||
| // legitimately carry tool-only payloads — e.g. a `[ComputerCall, | ||
| // ComputerCallOutput]` follow-up that resolves the prior assistant's | ||
| // safety-checked action. The prior conversation already established the | ||
| // user turn, so requiring another `Message`/`SimpleInputMessage` here | ||
| // would break the computer-use multi-turn flow (and analogous flows for | ||
| // function tools, MCP approvals, etc.). When neither continuation | ||
| // signal is set we still require at least one user-authored message OR | ||
| // a tool-item variant so that bad client requests (empty arrays, or | ||
| // `reasoning`-only payloads) continue to fail fast. | ||
| if request.previous_response_id.is_none() && request.conversation.is_none() { | ||
| if let ResponseInput::Items(items) = &request.input { | ||
| let has_valid_input = items.iter().any(|item| { | ||
| matches!( | ||
| item, | ||
| ResponseInputOutputItem::Message { .. } | ||
| | ResponseInputOutputItem::SimpleInputMessage { .. } | ||
| | ResponseInputOutputItem::FunctionToolCall { .. } | ||
| | ResponseInputOutputItem::FunctionCallOutput { .. } | ||
| | ResponseInputOutputItem::McpApprovalRequest { .. } | ||
| | ResponseInputOutputItem::McpApprovalResponse { .. } | ||
| | ResponseInputOutputItem::ComputerCall { .. } | ||
| | ResponseInputOutputItem::ComputerCallOutput { .. } | ||
| ) | ||
| }); | ||
|
|
||
| if !has_valid_input { | ||
| let mut e = ValidationError::new("input_missing_user_message"); | ||
| e.message = Some("Input items must contain at least one message".into()); | ||
| return Err(e); | ||
| if !has_valid_input { | ||
| let mut e = ValidationError::new("input_missing_user_message"); | ||
| e.message = Some( | ||
| "Input items must contain at least one message or tool item when neither \ | ||
| 'previous_response_id' nor 'conversation' is provided" | ||
| .into(), | ||
| ); | ||
| return Err(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenAI computer-use docs: does the legacy computer_use_previewtool still requiretruncation: "auto" on Responses create requests?
💡 Result:
Yes, the legacy computer_use_preview tool still requires truncation: "auto" on Responses create requests (/v1/responses). This is explicitly stated in the official OpenAI computer use documentation under the migration guide from the deprecated preview to the GA computer tool. The table contrasts the differences, noting that truncation: "auto" is required for the preview integration but not necessary for the new one. The model page confirms computer-use-preview is only usable in the Responses API and remains available (with a snapshot dated 2025-03-11), indicating it has not been shut down as of 2026-04-22. No deprecation shutdown date is listed for it, unlike other models.
Citations:
- 1: https://developers.openai.com/api/docs/guides/tools-computer-use
- 2: https://developers.openai.com/docs/guides/tools-computer-use
- 3: https://developers.openai.com/api/docs/models/computer-use-preview
- 4: https://developers.openai.com/api/docs/guides/tools-computer-use/
🏁 Script executed:
# Find the ResponseTool enum definition
rg "enum ResponseTool" -A 20 --type rustRepository: lightseekorg/smg
Length of output: 1300
🏁 Script executed:
# Search for any existing validation of computer_use_preview
rg "computer_use_preview|ComputerUsePreview" --type rust -iRepository: lightseekorg/smg
Length of output: 2945
🏁 Script executed:
# Examine the validate_responses_cross_parameters function
rg "fn validate_responses_cross_parameters" -A 50 --type rustRepository: lightseekorg/smg
Length of output: 4323
🏁 Script executed:
# Get the full validate_responses_cross_parameters function
rg "fn validate_responses_cross_parameters" -A 150 --type rustRepository: lightseekorg/smg
Length of output: 11986
🏁 Script executed:
# Check if there's any existing truncation validation for computer_use_preview
rg "truncation.*computer_use_preview|computer_use_preview.*truncation" --type rust -iRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Also search for any truncation-related validation in responses.rs
rg "truncation" crates/protocols/src/responses.rs -B 2 -A 2Repository: lightseekorg/smg
Length of output: 496
Add validation requiring truncation: "auto" when using computer_use_preview tool.
The validator currently accepts requests with the computer_use_preview tool without enforcing OpenAI's documented requirement that truncation must be set to "auto". This will cause requests to pass validation but fail at the OpenAI Responses API, creating a downstream error.
Suggested fix
fn validate_responses_cross_parameters(request: &ResponsesRequest) -> Result<(), ValidationError> {
// 1. Validate tool_choice requires tools (enhanced)
validate_tool_choice_with_tools(request)?;
+ let uses_computer_use_preview = request
+ .tools
+ .as_ref()
+ .is_some_and(|tools| {
+ tools
+ .iter()
+ .any(|tool| matches!(tool, ResponseTool::ComputerUsePreview(_)))
+ });
+ if uses_computer_use_preview && !matches!(request.truncation, Some(Truncation::Auto)) {
+ let mut e = ValidationError::new("computer_use_preview_requires_truncation_auto");
+ e.message = Some("`computer_use_preview` requires `truncation: \"auto\"`.".into());
+ return Err(e);
+ }
+
// 2. Validate top_logprobs requires include field
if request.top_logprobs.is_some() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/protocols/src/responses.rs` around lines 1968 - 2005, The validator
must reject requests that include computer-use items when truncation is not set
to "auto": update the validation after the items check to scan items for
ResponseInputOutputItem::ComputerCall and
ResponseInputOutputItem::ComputerCallOutput and, if any are present, ensure
request.truncation is Some("auto") (or the Truncation::Auto enum variant used in
this crate); if not, return a ValidationError (e.g.,
"truncation_must_be_auto_for_computer_use_preview") with a message telling the
caller to set truncation to "auto". Use the existing request,
request.truncation, and the ResponseInputOutputItem::ComputerCall /
::ComputerCallOutput symbols to locate where to add this check.
There was a problem hiding this comment.
Declining as scope-creep — enforcing truncation: "auto" for computer_use_preview requests is server-side validation/behavior, not a protocol-schema concern. Per the audit playbook §7, this PR is protocol-only. The Rust schema already represents truncation as an optional enum; runtime enforcement of 'preview tool requires auto-truncation' belongs in a router/R task.
There was a problem hiding this comment.
@slin1237, understood — acknowledged and noted. I'll leave the cross-field truncation enforcement for the router/R task as you described.
✏️ Learnings added
Learnt from: slin1237
Repo: lightseekorg/smg PR: 1305
File: crates/protocols/src/responses.rs:0-0
Timestamp: 2026-04-22T20:29:10.902Z
Learning: In repo lightseekorg/smg, `crates/protocols/src/responses.rs`: enforcing `truncation: "auto"` when the `computer_use_preview` tool is present is intentionally NOT done in the protocol-level validator (`validate_responses_cross_parameters`). Per audit playbook §7, protocol PRs are schema-only; runtime cross-field enforcement of `computer_use_preview` requiring `truncation: "auto"` belongs in a downstream router/R task. Do not re-flag the absence of this check as a protocol-layer bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: slin1237
Repo: lightseekorg/smg PR: 543
File: clients/rust/src/api/responses.rs:53-61
Timestamp: 2026-02-26T16:01:45.180Z
Learning: In the Rust client (clients/rust/src/api/responses.rs), the list_input_items method correctly returns serde_json::Value because the /v1/responses/{response_id}/input_items endpoint returns variable-structure JSON without a fixed schema, and no typed struct exists in the protocols crate for this response.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 1305
File: crates/protocols/src/responses.rs:842-883
Timestamp: 2026-04-22T05:59:40.769Z
Learning: In repo lightseekorg/smg, `ResponseInputOutputItem::ComputerCall` / `::ComputerCallOutput` and `ResponseOutputItem::ComputerCall` / `::ComputerCallOutput` in `crates/protocols/src/responses.rs` intentionally duplicate their field sets rather than sharing a payload struct with `#[serde(flatten)]`. The input-side variants coexist with the untagged `SimpleInputMessage` fallback arm (requiring per-variant tag precision), while `#[serde(flatten)]` would break the per-variant `#[serde(rename = "...")]` discriminators and cause JSON roundtrip divergence from the SDK shape. Drift is mitigated by inline `Spec (openai-responses-api-spec.md §…)` doc comments and the round-trip tests `test_computer_call_input_item_round_trip`, `test_computer_call_output_item_round_trip`, and `test_computer_call_pending_safety_checks_always_serialized`. Do not suggest extracting these into shared structs.
Learnt from: CatherineSue
Repo: lightseekorg/smg PR: 791
File: model_gateway/src/routers/grpc/harmony/stages/preparation.rs:118-124
Timestamp: 2026-03-17T20:14:15.295Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/grpc/harmony/stages/preparation.rs (prepare_chat): The condition `if constraint.is_some() && body_ref.response_format.is_some()` that clears `response_format` only fires when `response_format` was the source of the structural-tag constraint. Protocol-level validation and the previous pipeline stage ensure that a request carrying both a tool constraint and a `response_format` is rejected before reaching this stage. Therefore the comment "If response_format was converted to a structural tag, clear it..." accurately describes the runtime behavior.
Learnt from: TingtingZhou7
Repo: lightseekorg/smg PR: 1057
File: model_gateway/src/routers/openai/mcp/tool_loop.rs:856-885
Timestamp: 2026-04-08T00:08:05.944Z
Learning: In repo lightseekorg/smg, `sanitize_builtin_tool_arguments` in `model_gateway/src/routers/openai/mcp/tool_loop.rs` intentionally drops image-generation options (size, quality, background, output_format, compression) when handling `ResponseFormat::ImageGenerationCall`, keeping only `model` (hardcoded `IMAGE_MODEL`) and `revised_prompt`. This is a deliberate scoped decision for the initial image-generation tool integration; per-option overrides/defaults are planned for a follow-up PR. Do not flag the truncation as a bug or request preservation of extra fields.
Learnt from: CatherineSue
Repo: lightseekorg/smg PR: 1146
File: crates/grpc_client/src/sglang_scheduler.rs:515-534
Timestamp: 2026-04-15T06:15:49.617Z
Learning: In repo lightseekorg/smg, `crates/grpc_client/src/sglang_scheduler.rs` `build_constraint_for_chat`: The `tool_call_constraint: Option<(String, String)>` argument is always produced by the gRPC preparation stage (never raw user input). The preparation stage guarantees only valid constraint types ("structural_tag", "json_schema", "ebnf", "regex") are emitted. Do not flag the absence of unknown-type validation in the "drop" (non-empty constraints) branch as a bug; the internal contract ensures an unknown type will never reach that code path.
Learnt from: CatherineSue
Repo: lightseekorg/smg PR: 1090
File: model_gateway/src/routers/grpc/regular/stages/chat/preparation.rs:184-204
Timestamp: 2026-04-13T23:26:24.570Z
Learning: In repo lightseekorg/smg, `model_gateway/src/routers/grpc/regular/stages/chat/preparation.rs` (and the messages preparation counterpart): `generate_tool_constraint` intentionally receives only `ctx.components.configured_tool_parser` (set via `--tool-call-parser` CLI flag) with no model-based auto-detection fallback. Structural tag constraints (Mistral, KimiK2) require explicit `--tool-call-parser` opt-in because model IDs can be aliases that map to the same underlying model, making auto-detection by model name unreliable. Do not flag the absence of a model-based parser resolver fallback in `generate_tool_constraint` calls as a bug.
Learnt from: CatherineSue
Repo: lightseekorg/smg PR: 1090
File: crates/tool_parser/src/factory.rs:0-0
Timestamp: 2026-04-13T23:25:52.882Z
Learning: In repo lightseekorg/smg, `crates/tool_parser/src/factory.rs` `ParserRegistry::generate_tool_constraint`: The method does NOT internally filter `tools` by `tool_choice`. Tool filtering is the caller's responsibility — the messages preparation stage pre-filters the tool list before calling this function. For `ToolChoice::Function`, the json_schema fallback uses `tools[0]` which matches pre-PR behavior (caller ensures the target tool is at index 0 or passes the full list knowing the model-native framing handles selection). Structural tag builders (`build_structural_tag`) intentionally receive the full tool list because xgrammar's `triggered_tags` format must enumerate all allowed tools. Do not flag the absence of in-function tool filtering as a bug.
Learnt from: vschandramourya
Repo: lightseekorg/smg PR: 915
File: model_gateway/src/routers/grpc/client.rs:387-423
Timestamp: 2026-03-26T17:06:14.307Z
Learning: In repo lightseekorg/smg, in `model_gateway/src/routers/grpc/client.rs` and the corresponding backend builders (`crates/grpc_client/src/sglang_scheduler.rs`, `vllm_engine.rs`, `trtllm_service.rs`): The per-backend divergence in handling `CompletionRequest.max_tokens == None` is intentional. SGLang and vLLM pass `None` through to their proto builders, while TRT-LLM falls back to `16`. This matches the pre-existing per-backend pattern used in the chat/messages request builders. Do not flag this divergence as a bug or request normalization at the `build_completion_request` dispatcher layer in `client.rs`.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/core/token_bucket.rs:58-63
Timestamp: 2026-02-21T02:30:51.443Z
Learning: For lint-only/Clippy enforcement PRs in this repository, avoid introducing behavioral changes (e.g., new input validation or logic changes). Treat such PRs as non-functional changes and plan a separate follow-up issue/PR for hardening or behavior changes. This applies broadly to Rust files across the repo; during review, focus on lint/style corrections and clearly note any intentional exceptions.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 1163
File: model_gateway/src/routers/common/mcp_utils.rs:56-67
Timestamp: 2026-04-17T18:06:31.006Z
Learning: In repo lightseekorg/smg, `inject_mcp_output_items` in `model_gateway/src/routers/common/mcp_utils.rs` (used by gRPC regular and Harmony response paths) does NOT need to filter `existing` output items (from `std::mem::take(output)`) with `is_client_visible_output_item` before appending them back. Unlike the OpenAI path (`inject_client_visible_mcp_output_items` in `crates/mcp/src/core/session.rs`), the gRPC response paths do not pre-populate `response.output` with internal `FunctionToolCall` entries before this helper is called, so there is no internal-item leakage risk. The e2e assertions in `e2e_test/responses/test_tools_call.py` confirm this. Do not apply the OpenAI-path filtering requirement to `inject_mcp_output_items` in the gRPC paths.
Learnt from: key4ng
Repo: lightseekorg/smg PR: 1006
File: crates/tool_parser/src/parsers/deepseek31.rs:162-181
Timestamp: 2026-04-01T04:14:46.469Z
Learning: In repo lightseekorg/smg, `crates/tool_parser/src/parsers/deepseek31.rs` (and the analogous V3 parser `crates/tool_parser/src/parsers/deepseek.rs` lines 186-197): The `parse_incremental` method does NOT split off a plain-text prefix before the first tool marker within the same chunk. This is intentional because the gRPC streaming path delivers tokens individually, so normal text content and tool-call markers always arrive in separate chunks — a prefix and a tool marker will never coexist in the same chunk. Do not flag the absence of a within-chunk prefix-split as a bug; the `test_deepseek31_streaming_text_before_tools` test covers the realistic multi-chunk case.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 1250
File: model_gateway/src/routers/openai/responses/history.rs:343-398
Timestamp: 2026-04-20T09:00:17.756Z
Learning: In repo lightseekorg/smg, `mcp_call_output_to_upstream_items` in `model_gateway/src/routers/openai/responses/history.rs` intentionally uses a redundant `item.get("output")` double-lookup and does not preserve the `error` field from stored `mcp_call` items during upstream replay (producing `"null"` instead). This is a deliberate scoping decision in PR `#1250`: the exercised paths always carry an `output` value, and widening replay behavior to handle missing-output/error cases is deferred to a follow-up PR. Do not re-flag the redundant lookup or the silent error discard as blocking issues until that follow-up work is done.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 491
File: protocols/src/responses.rs:60-66
Timestamp: 2026-02-21T02:23:25.181Z
Learning: In protocols/src/responses.rs, the RequireApproval enum intentionally only supports string forms (Always/Never) for now. The object form with filters (read_only, tool_names) for per-tool approval configuration is planned for later implementation.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: protocols/src/responses.rs:928-931
Timestamp: 2026-02-21T02:36:00.882Z
Learning: In Rust code across the repository, use the marker INVARIANT: to document assumptions in safe code. Reserve SAFETY: for explaining why unsafe blocks are sound. This improves clarity of invariants and safety reasoning. Example reference: protocols/src/responses.rs near validate_tool_choice_with_tools().
Learnt from: CatherineSue
Repo: lightseekorg/smg PR: 638
File: grpc_servicer/smg_grpc_servicer/vllm/servicer.py:371-406
Timestamp: 2026-03-05T04:48:49.033Z
Learning: In repo lightseekorg/smg, `grpc_servicer/smg_grpc_servicer/vllm/servicer.py` (`_build_preprocessed_mm_inputs`): multimodal metadata fields (`flat_keys`, `batched_keys`, `mm_placeholders`, `model_specific_tensors`) are produced exclusively by the internal Rust router and are treated as trusted data. Defensive input-validation guards (e.g., checking flat_keys targets exist in hf_dict, validating PlaceholderRange bounds) are intentionally omitted to avoid hot-path overhead. Do not flag missing validation on these fields in future reviews.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 1163
File: model_gateway/src/routers/common/mcp_utils.rs:20-46
Timestamp: 2026-04-16T01:18:25.172Z
Learning: In repo lightseekorg/smg, the `mcp_list_tools` resume deduplication in `model_gateway/src/routers/common/mcp_utils.rs` (`extract_mcp_list_tools_labels` + `mcp_list_tools_bindings_to_emit`) intentionally uses `server_label` alone as the dedup key, matching the pre-existing OpenAI path behavior. The gRPC paths (regular and Harmony) were aligned to this same label-only semantic in PR `#1163`. A binding-fingerprint approach (including `server_key`, `server_url`, and `allowed_tools`) is a known future improvement but is intentionally out of scope for this PR. Do not flag label-only dedup in these functions as a bug without referencing this prior context.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 807
File: model_gateway/src/routers/openai/responses/streaming.rs:821-855
Timestamp: 2026-03-18T21:57:03.433Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/openai/responses/streaming.rs: The early `return` statements inside `handle_streaming_with_tool_interception` (on `tx` send failures in `forward_streaming_event`, `send_mcp_list_tools_events`, and the `is_in_progress`/`mcp_list_tools_sent` branches) are pre-existing behavior that predates PR `#807`. They cause the persistence phase (final response / conversation-backed storage writes at the end of the tool loop) to be skipped when the client disconnects mid-stream with `store=true` or a conversation-backed request. This is a known pre-existing gap in the MCP streaming path, not a regression introduced by the storage context header changes in PR `#807`.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 1174
File: crates/mcp/src/core/orchestrator.rs:1097-1131
Timestamp: 2026-04-17T06:48:35.192Z
Learning: In repo lightseekorg/smg, `execute_tool_entry_result` in `crates/mcp/src/core/orchestrator.rs`: `record_call_end` is intentionally called for both `ToolExecutionResult::Executed` and `ToolExecutionResult::PendingApproval` to preserve pre-refactor metric shape (pending approval closes out the current tool-handling attempt). For `PendingApproval`, `succeeded = false` was a bug fixed in PR `#1174` so it no longer inflates error rates, but a dedicated `record_call_pending_approval` counter and full lifecycle split (only recording completion when approval is resolved and execution runs) are deferred to a follow-up PR. Do not flag the absence of a dedicated pending-approval metric as a blocking issue until that follow-up work is done.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 1244
File: crates/protocols/src/builders/responses/response.rs:126-143
Timestamp: 2026-04-20T16:55:01.974Z
Learning: In repo lightseekorg/smg, `completed_at: Option<i64>` on `ResponsesResponse` (added in PR `#1244`, `crates/protocols/src/builders/responses/response.rs`) is intentionally NOT populated by any terminal-response builder in PR `#1244`. Wiring the gRPC regular (`conversions.rs`, `streaming.rs`) and Harmony (`processor.rs`, `non_streaming.rs`) terminal builders to call `.completed_at(...)` before `.build()` is explicitly deferred to BGM-PR-07 (`Worker Execution Core + Retry + Cancellation`) per `2026-04-17-background-mode-task-breakdown.md:170-193`. Do not re-flag `completed_at: None` in terminal responses as a missing population until BGM-PR-07 lands.
Learnt from: XinyueZhang369
Repo: lightseekorg/smg PR: 399
File: protocols/src/interactions.rs:505-509
Timestamp: 2026-02-19T03:08:50.192Z
Learning: In code reviews for Rust projects using the validator crate (v0.20.0), ensure that custom validation functions for numeric primitive types (e.g., f32, i32, u32, i16, etc.) accept the value by value, not by reference. Example: fn validate(value: f32) { ... }. The validator derive macro has a hardcoded list of numeric types that are passed by value, while all other types are passed by reference. Apply this guideline whenever validating numeric fields to align with the derive macro behavior.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: mesh/src/sync.rs:83-83
Timestamp: 2026-02-21T02:37:01.416Z
Learning: General Rust formatting rule: format! with implicit captures only supports simple identifiers, not full expressions like {state.model_id}. For cases where you want to interpolate a field or expression, bind the value first and interpolate the binding, e.g., let model_id = &state.model_id; and then use format!("policy:{}", model_id). In the specific file mesh/src/sync.rs, prefer format!("policy:{}", state.model_id) or bind to a local variable if you need named interpolation, to keep clarity and avoid unintended captures.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 807
File: model_gateway/src/middleware.rs:61-81
Timestamp: 2026-03-18T21:32:00.041Z
Learning: In Rust code using the http crate, HeaderMap::get() is effectively case-insensitive because HeaderName normalizes keys to lowercase on insertion and lookup. Do not require or perform explicit .to_lowercase() before HeaderMap::get() calls. Mark as not a concern for case-sensitivity in lookups; only consider normalization when inserting or comparing via HeaderName, not in lookups.
Learnt from: key4ng
Repo: lightseekorg/smg PR: 867
File: tui/src/app.rs:798-813
Timestamp: 2026-03-22T20:13:55.778Z
Learning: In this repo (lightseekorg/smg), treat the workspace `Cargo.toml`’s `package.rust-version` (MSRV) as the source of truth (e.g., `rust-version = "1.85"`). When reviewing Rust changes, do not flag usage of Rust language/library features that were stabilized on or before the MSRV (e.g., `Option::is_none_or`, stabilized in 1.82, is compatible with an MSRV of 1.85). Always verify the MSRV from the workspace `Cargo.toml` rather than relying on issue templates.
cf7c0ea to
1c7f594
Compare
|
Branch rewritten to schema-only scope. Removed defensive guardrails from router files; kept only forced-cascade match arms. Spec ref: SDK v2.8.1 types/responses/computer_tool.py. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c7f594cee
ℹ️ 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".
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| keys: Option<Vec<String>>, |
There was a problem hiding this comment.
When a computer_call action arrives as {"type":"double_click", ..., "keys": null}, this field deserializes to None and is then dropped during re-serialization because of skip_serializing_if = "Option::is_none". That means history replay/forwarding no longer round-trips the original wire shape and can violate strict consumers that treat keys as a required-but-nullable field for double_click actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining — see my reply on the companion P1 comment (#3125624652). SDK v2.8.1 ActionDoubleClick has no keys field at all, so there is no "null-keys preservation" spec behavior to implement. The \"keys\": null case degrades cleanly today (deserializes to None, serializes as absent), which is the correct treatment for a field that isn't in the upstream schema.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/protocols/src/responses.rs (3)
388-395:⚠️ Potential issue | 🟠 Major
computer_use_previewstill needs thetruncation: "auto"validator.Adding the tool variant here is not enough. Requests using
computer_use_previewstill passvalidate_responses_cross_parameters()withouttruncation: "auto", so callers get a downstream API error instead of a local validation failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/protocols/src/responses.rs` around lines 388 - 395, The ComputerUsePreview enum variant (ComputerUsePreview(ComputerUsePreviewTool)) is missing a cross-parameter validation that enforces truncation: "auto"; update validate_responses_cross_parameters() to check for the presence of the ComputerUsePreview tool (match on the ComputerUsePreview variant or inspect tool.type == "computer_use_preview") and return a validation error unless the associated truncation parameter equals "auto", ensuring callers get a local validation failure instead of a downstream API error.
814-820:⚠️ Potential issue | 🟠 Major
ComputerSafetyCheckis too strict for upstream payloads.
codeandmessageare optional in the computer-call safety-check payload. Keeping them as requiredStringvalues will reject validcomputer_call/computer_call_outputitems whenever either field is omitted.Proposed fix
pub struct ComputerSafetyCheck { pub id: String, - pub code: String, - pub message: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub code: Option<String>, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub message: Option<String>, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/protocols/src/responses.rs` around lines 814 - 820, The ComputerSafetyCheck struct is too strict: change the types of the fields code and message from String to Option<String> in the ComputerSafetyCheck struct (keep the derives and #[serde(deny_unknown_fields)]), add #[serde(default, skip_serializing_if = "Option::is_none")] to those fields if you want to omit them when serializing, and update any callers/consumers that read ComputerSafetyCheck (e.g., places that access .code or .message) to handle Option<String> (unwrap/propagate/default as appropriate).
2287-2288:⚠️ Potential issue | 🔴 CriticalTool-only computer continuations are still blocked.
Allowing these variants here does not unblock the resume flow. Line 2197 still requires a
Message/SimpleInputMessage, soprevious_response_id + [{ "type": "computer_call_output", ... }]is rejected before routing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/protocols/src/responses.rs` around lines 2287 - 2288, The resume flow is still blocked because permitting ResponseInputOutputItem::ComputerCall and ::ComputerCallOutput in the enum match does not change the earlier validator that requires a Message/SimpleInputMessage when composing previous_response_id + continuation; update the validator that checks for Message and SimpleInputMessage to also accept computer-only continuations by allowing ResponseInputOutputItem::ComputerCall and ResponseInputOutputItem::ComputerCallOutput (or their serialized equivalents) as valid continuation entries in the previous_response_id composition logic (the code path that currently rejects non-Message continuations), ensuring the resume routing accepts previous_response_id + [{ "type": "computer_call_output", ... }].
🤖 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 441-442: The new ResponseTool variants (ResponseTool::Computer and
ResponseTool::ComputerUsePreview) are mapped to the strings "computer" and
"computer_use_preview" but BUILTIN_TOOLS was not updated, so
has_custom_tools(...) misclassifies them; update the BUILTIN_TOOLS collection to
include "computer" and "computer_use_preview" (or canonicalize to whatever
string keys has_custom_tools checks) so that requests using
ResponseTool::Computer / ResponseTool::ComputerUsePreview are treated as
built-ins; touch the BUILTIN_TOOLS definition and any canonicalization logic
used by has_custom_tools to ensure the new tool names are recognized.
- Around line 726-728: The match arm handling
ResponseInputOutputItem::McpApprovalRequest, ::ComputerCall and
::ComputerCallOutput currently logs a variant-specific message ("Approval item
reached Harmony conversion"); change that to a neutral warning mentioning the
mixed/unsupported variants so it fits all three cases — update the log string in
the match arm that matches ResponseInputOutputItem::McpApprovalRequest { .. } |
ResponseInputOutputItem::ComputerCall { .. } |
ResponseInputOutputItem::ComputerCallOutput { .. } to something like
"Unsupported ResponseInputOutputItem variant reached Harmony conversion" (or
similar neutral phrasing) to improve debugging clarity.
---
Duplicate comments:
In `@crates/protocols/src/responses.rs`:
- Around line 388-395: The ComputerUsePreview enum variant
(ComputerUsePreview(ComputerUsePreviewTool)) is missing a cross-parameter
validation that enforces truncation: "auto"; update
validate_responses_cross_parameters() to check for the presence of the
ComputerUsePreview tool (match on the ComputerUsePreview variant or inspect
tool.type == "computer_use_preview") and return a validation error unless the
associated truncation parameter equals "auto", ensuring callers get a local
validation failure instead of a downstream API error.
- Around line 814-820: The ComputerSafetyCheck struct is too strict: change the
types of the fields code and message from String to Option<String> in the
ComputerSafetyCheck struct (keep the derives and #[serde(deny_unknown_fields)]),
add #[serde(default, skip_serializing_if = "Option::is_none")] to those fields
if you want to omit them when serializing, and update any callers/consumers that
read ComputerSafetyCheck (e.g., places that access .code or .message) to handle
Option<String> (unwrap/propagate/default as appropriate).
- Around line 2287-2288: The resume flow is still blocked because permitting
ResponseInputOutputItem::ComputerCall and ::ComputerCallOutput in the enum match
does not change the earlier validator that requires a Message/SimpleInputMessage
when composing previous_response_id + continuation; update the validator that
checks for Message and SimpleInputMessage to also accept computer-only
continuations by allowing ResponseInputOutputItem::ComputerCall and
ResponseInputOutputItem::ComputerCallOutput (or their serialized equivalents) as
valid continuation entries in the previous_response_id composition logic (the
code path that currently rejects non-Message continuations), ensuring the resume
routing accepts previous_response_id + [{ "type": "computer_call_output", ...
}].
🪄 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: 872178c2-dd8a-4228-b95f-d9769a2ed83e
📒 Files selected for processing (7)
.pre-commit-config.yamlcrates/mcp/src/core/session.rscrates/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
|
Bot-review triage after schema-only rewrite:
No code changes in this round; scope stays schema-only. |
|
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
1c7f594 to
cbf8a24
Compare
mergify
Bot
removed
the
needs-rebase
cbf8a24 to
81e63bb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81e63bbf71
ℹ️ 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".
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| keys: Option<Vec<String>>, |
There was a problem hiding this comment.
double_click.keys during deserialization
ComputerAction::DoubleClick currently uses Option<Vec<String>> with #[serde(default)], so payloads that omit keys deserialize successfully even though this field is required (nullable) in the computer action wire shape; missing and explicit null are collapsed into the same None value. This lets spec-invalid computer_call items pass validation and then reserialize without keys, which can fail when forwarded to strict consumers expecting the required field to be present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining — SDK v2.8.1 does not declare a keys field on ActionDoubleClick at all. Verified against openai==2.8.1, types/responses/response_computer_tool_call.py::ActionDoubleClick:
class ActionDoubleClick(BaseModel):
type: Literal["double_click"]
x: int
y: intNo keys field, required or optional. Requiring keys here would invent a field that isn't in SDK v2.8.1 and would reject valid SDK payloads. The current Option<Vec<String>> + skip_serializing_if = Option::is_none treats the field as a permissive extra, which is the safest stance without inventing schema beyond what the SDK declares. Per the task's 'never invent fields' rule and SDK-as-source-of-truth guidance, this stays as-is.
81e63bb to
4ce9995
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ce999530d
ℹ️ 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".
| pub code: String, | ||
| pub message: String, |
There was a problem hiding this comment.
The Responses schema allows safety-check entries to omit code and message (both are optional), but ComputerSafetyCheck requires them as non-optional Strings. That makes deserialization fail for otherwise valid computer_call / computer_call_output items that include only id, which can drop tool-state items during history loading or pass-through handling whenever upstream omits those fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ae610a4 — confirmed against SDK v2.8.1 PendingSafetyCheck:
class PendingSafetyCheck(BaseModel):
id: str
code: Optional[str] = None
message: Optional[str] = NoneChanged ComputerSafetyCheck.code/message from required String to Option<String> with skip_serializing_if = Option::is_none. Added regression test computer_safety_check_accepts_optional_code_and_message covering all four presence combinations.
4ce9995 to
fddd7c6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae610a4744
ℹ️ 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".
| ResponseInputOutputItem::ComputerCall { .. } => {} | ||
| ResponseInputOutputItem::ComputerCallOutput { .. } => {} |
There was a problem hiding this comment.
Allowing ResponseInputOutputItem::ComputerCall and ::ComputerCallOutput here means conversation requests can now include these items, but the persistence path still stores whole input payloads only for function_call/function_call_output (model_gateway/src/routers/common/persistence_utils.rs around store_whole_item and content selection). For computer input items, that logic writes content: [] and drops fields like call_id, output, and safety checks, so subsequent turns cannot reconstruct the tool state from conversation history. Please either persist these new input item types as whole JSON items or block them until persistence/history support is added.
Useful? React with 👍 / 👎.
|
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
Add the Responses API computer-use tool schema per spec and spec-mandated
SDK 2.8.1 types/responses/computer_tool.py:
- `ResponseTool::Computer` (unit variant, `{type: "computer"}`).
- `ResponseTool::ComputerUsePreview(ComputerUsePreviewTool)` with
`display_height`, `display_width`, `environment: "windows"|"mac"|
"linux"|"ubuntu"|"browser"`.
- `ComputerAction` tagged union covering all 9 spec actions
(click / double_click / drag / keypress / move / screenshot /
scroll / type / wait) with `MouseButton` and `ComputerCoordinate`.
- `ComputerCallStatus`, `ComputerSafetyCheck`,
`ComputerCallOutputContent::ComputerScreenshot { file_id?, image_url? }`.
- Input-item and output-item `ComputerCall` / `ComputerCallOutput`
mirroring the spec's `ResponseComputerToolCall` /
`ResponseComputerToolCallOutput` shapes.
`pending_safety_checks` is serialized even when empty (`#[serde(default)]`
without `skip_serializing_if`) to match the SDK contract where the field
is a non-`Optional` `List[PendingSafetyCheck]`.
Forced-cascade (compiler required the new variants be covered by
existing exhaustive matches): added minimum arms in
`crates/mcp/src/core/session.rs`,
`model_gateway/benches/routing_allocation_bench.rs`,
`model_gateway/src/routers/grpc/harmony/builder.rs`,
`model_gateway/src/routers/grpc/regular/responses/conversions.rs`,
and `model_gateway/src/routers/openai/responses/utils.rs`. No new
behaviour introduced — the new variants fall through the same branches
already used for the analogous image-generation variants.
Added `doubleclick` to the codespell allowlist so the
`ComputerAction::DoubleClick` variant name round-trips through the
snake_case hook.
Tests: 7 new round-trip tests in `crates/protocols/src/responses.rs`
covering `Computer`, `ComputerUsePreview`, every `ComputerAction`
variant, and input/output `ComputerCall` / `ComputerCallOutput` items.
Refs: T2
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Align ComputerSafetyCheck with OpenAI Python SDK v2.8.1 (types/responses/response_computer_tool_call.py::PendingSafetyCheck), which declares code and message as Optional[str] = None. The previous required String typing rejected spec-valid payloads that omit either field. - Change code: String -> Option<String> (skip_serializing_if Option::is_none) - Change message: String -> Option<String> (skip_serializing_if Option::is_none) - Add regression test computer_safety_check_accepts_optional_code_and_message covering all four combinations (neither/code-only/message-only/both) Protocol-only schema fix (no router behavior changes). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
87ddd43 to
369dc7c
Compare
mergify
Bot
removed
the
needs-rebase
1 participant
Summary
Implements audit task T2: `computer` / `computer_use_preview` tools on the Responses API.
What changed
Why
OpenAI Responses API spec defines `Computer { type: "computer" }` and `ComputerUsePreview { display_height, display_width, environment }` at `openai-responses-api-spec.md:437-438`, and the full `ComputerAction` wire union plus `ComputerCall` / `ComputerCallOutput` item types at `openai-responses-api-spec.md:143-165`. Without these, a spec-valid computer-use payload failed deserialization at the gateway boundary, blocking computer-use clients from routing through smg. The diff adds the surface types with `#[serde(tag = "type")]` discrimination mirroring the T1 FileSearch pattern.
Action taxonomy (spec line 150-159)
Test plan
Refs: T2
Closes audit task: T2
Summary by CodeRabbit
New Features
Behavior
Chores
Tests