feat(protocols): implement T5 local_shell tool + call/output items - #1338
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
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Model as Model
participant Gateway as Model Gateway
participant Router as Router
participant MCP as MCP Session
participant Client as Client
Model->>Gateway: emit Response (tool: local_shell, items)
Gateway->>Router: parse/allowlist tool & items
Router->>Gateway: mark local_shell input items unsupported / or pass through
Gateway->>MCP: forward response items
MCP->>MCP: is_client_visible_output_item -> true for local_shell outputs
MCP->>Client: deliver client-visible local_shell output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 71687e0068
ℹ️ 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(rename = "local_shell_call")] | ||
| LocalShellCall { |
There was a problem hiding this comment.
Adding ResponseInputOutputItem::LocalShellCall / LocalShellCallOutput expands this enum, but model_gateway/benches/routing_allocation_bench.rs (extract_text_for_routing_old) still uses an exhaustive match over ResponseInputOutputItem without these new arms or a wildcard, so bench targets now fail to compile when running cargo check --benches or cargo bench --no-run due to non-exhaustive patterns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Fixed in 456d1a5 by adding the forced-cascade arm (returns None to match sibling tool-call variants in the bench). Verified cargo clippy --all-targets --all-features -- -D warnings and cargo check --benches are clean locally.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Adds forced-cascade match arms for the new `ResponseInputOutputItem::LocalShellCall` and `LocalShellCallOutput` variants in `extract_text_for_routing_old` under `model_gateway/benches/routing_allocation_bench.rs`, so `cargo clippy --all-targets --all-features -- -D warnings` (CI lint step) and `cargo check --benches` pass with the T5 schema additions. Returns `None` to match every other tool-call/output arm in the bench — no behavior. Found by Codex Review on PR #1338. 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
Adds forced-cascade match arms for the new `ResponseInputOutputItem::LocalShellCall` and `LocalShellCallOutput` variants in `extract_text_for_routing_old` under `model_gateway/benches/routing_allocation_bench.rs`, so `cargo clippy --all-targets --all-features -- -D warnings` (CI lint step) and `cargo check --benches` pass with the T5 schema additions. Returns `None` to match every other tool-call/output arm in the bench — no behavior. Found by Codex Review on PR #1338. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
456d1a5 to
a03679b
Compare
mergify
Bot
removed
the
needs-rebase
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ResponseInputOutputItem::LocalShellCall { .. } | ||
| | ResponseInputOutputItem::LocalShellCallOutput { .. } => { | ||
| Err("Unsupported input item type".to_string()) |
There was a problem hiding this comment.
🟡 Nit: Missing warn! log — the adjacent ShellCall arm (L763-770) logs a warning before returning Err, but this arm silently returns an error. The log is useful for diagnosing unexpected items that reach the Harmony conversion path.
| ResponseInputOutputItem::LocalShellCall { .. } | |
| | ResponseInputOutputItem::LocalShellCallOutput { .. } => { | |
| Err("Unsupported input item type".to_string()) | |
| ResponseInputOutputItem::LocalShellCall { .. } | |
| | ResponseInputOutputItem::LocalShellCallOutput { .. } => { | |
| warn!( | |
| function = "parse_response_item_to_harmony_message", | |
| "LocalShell tool item reached Harmony conversion" | |
| ); | |
| Err("Unsupported input item type".to_string()) | |
| } |
| ResponseInputOutputItem::LocalShellCall { .. } | ||
| | ResponseInputOutputItem::LocalShellCallOutput { .. } => { | ||
| return Err("Unsupported input item type".to_string()); |
There was a problem hiding this comment.
🟡 Nit: Same as the Harmony builder — the adjacent ShellCall arm (L173-179) logs a warn! before returning, but this arm is silent. Adding the log keeps the two tool types consistent and preserves debuggability.
| ResponseInputOutputItem::LocalShellCall { .. } | |
| | ResponseInputOutputItem::LocalShellCallOutput { .. } => { | |
| return Err("Unsupported input item type".to_string()); | |
| ResponseInputOutputItem::LocalShellCall { .. } | |
| | ResponseInputOutputItem::LocalShellCallOutput { .. } => { | |
| warn!( | |
| function = "responses_to_chat", | |
| "LocalShell tool item reached chat conversion" | |
| ); | |
| return Err("Unsupported input item type".to_string()); | |
| } |
|
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
Adds forced-cascade match arms for the new `ResponseInputOutputItem::LocalShellCall` and `LocalShellCallOutput` variants in `extract_text_for_routing_old` under `model_gateway/benches/routing_allocation_bench.rs`, so `cargo clippy --all-targets --all-features -- -D warnings` (CI lint step) and `cargo check --benches` pass with the T5 schema additions. Returns `None` to match every other tool-call/output arm in the bench — no behavior. Found by Codex Review on PR #1338. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
a03679b to
68481ec
Compare
mergify
Bot
removed
the
needs-rebase
|
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
Adds the `local_shell` built-in Responses tool (unit) plus the
matching `local_shell_call` / `local_shell_call_output` items on
both `ResponseInputOutputItem` and `ResponseOutputItem`.
Protocol schema (`crates/protocols/src/responses.rs`):
- `ResponseTool::LocalShell` — unit variant `{ type: "local_shell" }`.
- `ResponseInputOutputItem::LocalShellCall { id, call_id, action,
status }` + `ResponseOutputItem::LocalShellCall` mirror.
- `ResponseInputOutputItem::LocalShellCallOutput { id, output, status? }`
+ `ResponseOutputItem::LocalShellCallOutput` mirror.
- `LocalShellExec::Exec { command, env, timeout_ms?, user?,
working_directory? }` as the `action` payload.
- `LocalShellCallStatus` enum (`in_progress | completed | incomplete`).
Spec (`.claude/_audit/openai-responses-api-spec.md`):
- L219-226 — LocalShellCall + LocalShellCallOutput shapes.
- L462 — `LocalShell { type: "local_shell" }` tool definition.
- L507-516 — listed under `output: array of ResponseOutputItem`.
Tests (`crates/protocols/tests/responses.rs`, 5 new):
- `local_shell_tool_round_trips_spec_shape`
- `local_shell_call_input_item_round_trips_spec_shape`
- `local_shell_call_input_item_round_trips_with_all_optionals`
- `local_shell_call_output_input_item_round_trips_spec_shape`
(covers both status-present and status-absent forms per SDK v2.8.1)
- `local_shell_output_item_variants_round_trip_spec_shape`
Forced-cascade match arms (schema-only, no behavior): 4 sites in
`smg` / `smg-mcp` (harmony builder tool-type projection,
harmony builder input item mapper, `responses_to_chat` converter,
openai `response_tool_to_value`) plus 2 sites inside the protocol
crate itself (`validate_input_item`, request text collector).
Refs: audit playbook §T5 (.claude/_audit/responses-api-gap-audit.md L503-515).
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Adds forced-cascade match arms for the new `ResponseInputOutputItem::LocalShellCall` and `LocalShellCallOutput` variants in `extract_text_for_routing_old` under `model_gateway/benches/routing_allocation_bench.rs`, so `cargo clippy --all-targets --all-features -- -D warnings` (CI lint step) and `cargo check --benches` pass with the T5 schema additions. Returns `None` to match every other tool-call/output arm in the bench — no behavior. Found by Codex Review on PR #1338. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
68481ec to
69611ba
Compare
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69611ba8d6
ℹ️ 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".
| ResponseTool::Shell(_) => "shell", | ||
| ResponseTool::ApplyPatch => "apply_patch", | ||
| // T5 schema-only: forced-cascade arm, no behavior. | ||
| ResponseTool::LocalShell => "local_shell", |
There was a problem hiding this comment.
Adding ResponseTool::LocalShell => "local_shell" here feeds has_custom_tools(&tool_types), but BUILTIN_TOOLS in the same file does not include "local_shell". For requests that only declare the new built-in tool, this incorrectly sets with_custom_tools = true, which changes prompt construction (keeps commentary channel and injects a developer message despite no custom/function tools). This is a behavior regression for local-shell-only requests and should be fixed by adding local_shell to the built-in classification path.
Useful? React with 👍 / 👎.
1 participant
Summary
Implements audit task T5: adds the
local_shellbuilt-in tool onResponseToolplus the matchinglocal_shell_call/local_shell_call_outputitems on bothResponseInputOutputItemandResponseOutputItem.What changed
crates/protocols/src/responses.rs:ResponseTool::LocalShell— unit variant ({ type: "local_shell" }).ResponseInputOutputItem::LocalShellCall { id, call_id, action, status }+ mirror onResponseOutputItem.ResponseInputOutputItem::LocalShellCallOutput { id, output, status? }+ mirror onResponseOutputItem.LocalShellExec::Exec { command, env, timeout_ms?, user?, working_directory? }andLocalShellCallStatus(in_progress | completed | incomplete).validate_input_item, request text collector).crates/protocols/tests/responses.rs: 5 integration round-trip tests (tool, input call, input call with all optionals, input output both status-present/absent, output-item mirror).crates/mcp/src/core/session.rs,model_gateway/src/routers/grpc/harmony/builder.rs,model_gateway/src/routers/grpc/regular/responses/conversions.rs,model_gateway/src/routers/openai/responses/utils.rs: 4 forced-cascade match arms for the newly added variants, each one line / schema-only (no behavior).Why
Per the Responses API spec (
.claude/_audit/openai-responses-api-spec.md):LocalShellCall/LocalShellCallOutputitem shapes.LocalShell { type: "local_shell" }tool.output: array of ResponseOutputItem.Status-optional on
LocalShellCallOutputmirrors the OpenAI Python SDK v2.8.1 (openai==2.8.1,types/responses/response_input_item_param.py::LocalShellCallOutput).Verification
cargo check -p openai-protocol --testspassescargo test -p openai-protocol --test responsespasses (70 tests, incl. 5 new)cargo check -p smg --libpassescargo fmt --all --checkpassescargo clippy -p openai-protocol -p smg --lib --tests -- -D warningsclean.claude/_audit/openai-responses-api-spec.md§LocalShellCall L219-226, §tools L462Blast radius
crates/protocols/src/responses.rs,crates/protocols/tests/responses.rs, plus minimal forced-cascade arms incrates/mcp/src/core/session.rs,model_gateway/src/routers/grpc/harmony/builder.rs,model_gateway/src/routers/grpc/regular/responses/conversions.rs,model_gateway/src/routers/openai/responses/utils.rs.Out of scope
local_shell_callbeyond the forced-cascade placeholders.Refs: T5 (
.claude/_audit/responses-api-gap-audit.mdL503-515).Summary by CodeRabbit
New Features
Behavior
Tests