feat(protocols): implement T9 namespace tool grouping - #1337
Conversation
Responses API spec §tools L475 defines a `Namespace` tool that groups
related `Function` / `Custom` tools under a shared name + description.
Inner elements share the top-level shape but are restricted to those
two variants — nested namespaces and hosted/built-in tools are not
permitted as elements.
Protocol changes (`crates/protocols/src/responses.rs`):
- Add `ResponseTool::Namespace { description, name, tools:
Vec<NamespaceTool> }` tagged `#[serde(rename = "namespace")]`.
- Add a dedicated `NamespaceTool = Function(FunctionTool) |
Custom(CustomTool)` enum rather than reusing `ResponseTool`. Using
`ResponseTool` would structurally allow recursive `Namespace` nesting
and hosted tools as namespace elements — both explicitly forbidden
by spec.
Forced-cascade router arms (no scope bleed):
- `response_tool_to_value` (openai/responses/utils.rs) — route
`Namespace` through `serde_json::to_value` so the full payload
round-trips back to the client, mirroring how `Custom` / hosted
tools are handled.
- Harmony `tool_types` mapping (grpc/harmony/builder.rs) — emit
`"namespace"` to match the spec discriminator.
Integration tests (`crates/protocols/tests/responses.rs`, 4 new):
- `test_namespace_tool_with_function_round_trip` — namespace carrying
a single `Function` element.
- `test_namespace_tool_with_custom_text_format_round_trip` — namespace
carrying a `Custom` tool with `Text` format.
- `test_namespace_tool_with_custom_grammar_format_round_trip` —
namespace carrying a `Custom` tool with `Grammar` format, exercised
for both `lark` and `regex` syntaxes.
- `test_namespace_tool_mixed_elements_round_trip` — single namespace
mixing `Function` and `Custom` elements.
Refs: T9 (.claude/_audit/responses-api-gap-audit.md §T9, L568-578)
Spec: .claude/_audit/openai-responses-api-spec.md L475
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
github-actions
Bot
added
grpc
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| #[serde(rename = "namespace")] | ||
| Namespace { | ||
| /// Human-readable description surfaced to the model alongside the group. | ||
| description: String, | ||
| /// Stable identifier the model uses to address the namespace in | ||
| /// `function_call` / `custom_tool_call` items (via the `namespace` field). | ||
| name: String, | ||
| /// Tools in this namespace. Spec restricts elements to `Function` or | ||
| /// `Custom`; the dedicated [`NamespaceTool`] enum prevents nested | ||
| /// namespaces and hosted-tool leakage that the parent `ResponseTool` | ||
| /// enum would otherwise allow. | ||
| tools: Vec<NamespaceTool>, | ||
| }, |
There was a problem hiding this comment.
🟡 Nit: The Namespace variant uses inline struct fields, so it doesn't get #[serde(deny_unknown_fields)] — unlike peer variants like Custom(CustomTool) and FunctionTool where the wrapper struct rejects unexpected keys. A payload like {"type": "namespace", "name": "x", "description": "y", "tools": [], "bogus": 1} will silently succeed here but would fail for Custom.
Consider extracting a NamespaceToolDef struct (with deny_unknown_fields) and using Namespace(NamespaceToolDef) to stay consistent with the other arms. Not blocking — this can land as-is and be tightened later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/tests/responses.rs`:
- Around line 1457-1647: Add fail-fast tests that assert deserialization fails
when a Namespace's tools array contains disallowed child types: create two tests
(e.g., test_namespace_tool_rejects_nested_namespace and
test_namespace_tool_rejects_hosted_tool) that build payloads where
ResponseTool::Namespace has a tools entry with "type":"namespace" and another
with a hosted tool type like "file_search", then call
serde_json::from_value(payload) and assert it returns an Err (i.e.,
deserialization fails). Locate the namespace parsing logic via ResponseTool and
NamespaceTool types to ensure these tests cover the invariant that
namespace.tools only allow Function or Custom.
🪄 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: 375894c8-91bb-4c10-9995-3275f027d86e
📒 Files selected for processing (4)
crates/protocols/src/responses.rscrates/protocols/tests/responses.rsmodel_gateway/src/routers/grpc/harmony/builder.rsmodel_gateway/src/routers/openai/responses/utils.rs
There was a problem hiding this comment.
Clean, well-scoped protocol addition. The NamespaceTool enum correctly prevents recursive nesting and hosted-tool leakage per spec. Tests cover all element-type combinations with round-trip verification. One minor nit posted about deny_unknown_fields consistency on the inline struct fields.
0 🔴 Important · 1 🟡 Nit · 0 🟣 Pre-existing
Two follow-ups from PR #1337 review (Claude nit + CodeRabbit nitpick). 1. Extract `NamespaceToolDef` struct (`crates/protocols/src/responses.rs`) Claude flagged that the inline struct-variant form of `ResponseTool::Namespace { description, name, tools }` cannot carry `#[serde(deny_unknown_fields)]`, unlike sibling tuple variants such as `Custom(CustomTool)` / `Function(FunctionTool)` whose wrapper structs reject unknown keys. A payload with a bogus top-level key (e.g. `{"type": "namespace", "name": "x", "bogus": 1, ...}`) was silently accepted — inconsistent with how the rest of the enum behaves. Switched to `ResponseTool::Namespace(NamespaceToolDef)` with `#[serde(deny_unknown_fields)]` on the dedicated struct. Updates propagate to the two forced-cascade router arms plus the matching test destructurings. 2. Add fail-fast tests for disallowed namespace child types (`crates/protocols/tests/responses.rs`) CodeRabbit requested negative tests locking in the spec invariant that namespace.tools may contain only Function or Custom. Added two deserialization-failure tests: - `test_namespace_tool_rejects_nested_namespace_element` — nested `{"type": "namespace", ...}` must fail. - `test_namespace_tool_rejects_hosted_tool_element` — hosted tools like `file_search` must fail. Tests: `cargo test -p openai-protocol --test responses` → 65 passed (2 new negative, 4 round-trip from the prior commit). Gates: clippy clean, fmt clean. Refs: review comments on #1337 (claude[bot], coderabbitai[bot]). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Thanks for the review! Addressed both items in 3743cce:
Namespace suite now runs 6 tests (4 round-trip + 2 negative), 65 passing in total. Clippy + fmt clean. |
![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: 3743ccedda
ℹ️ 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 enum NamespaceTool { | ||
| /// Function tool — same shape as [`ResponseTool::Function`]. | ||
| #[serde(rename = "function")] | ||
| Function(FunctionTool), |
There was a problem hiding this comment.
Do not reuse FunctionTool for namespace members: this path flattens into common::Function (which requires parameters and has no defer_loading field), so namespace tools like {"type":"namespace",...,"tools":[{"type":"function","name":"lookup","defer_loading":true}]} cannot be represented faithfully. As a result, valid namespace function definitions either fail deserialization (missing required schema fields) or silently lose namespace-specific function metadata, which breaks deferred namespace/tool-search workflows.
Useful? React with 👍 / 👎.
1 participant
Description
Problem
Task T9 of the Responses API gap audit: the
openai-protocolcrate hasno representation for the
Namespacetool defined in the Responses APIspec. Requests carrying a
{"type": "namespace", ...}tool cannot bedeserialized, so namespace-based tool grouping is unreachable end-to-end.
Spec (.claude/_audit/openai-responses-api-spec.md §tools L475):
Audit entry: .claude/_audit/responses-api-gap-audit.md §T9 (L568-578).
Solution
Add the
Namespacevariant toResponseTooland a dedicated nestedNamespaceToolenum that restricts elements toFunctionorCustomper spec. Wire the two forced-cascade router call sites so the existing
smglib still compiles. Scope is protocol-only (no guardrails, norouter behavior beyond the exhaustive-match arms that the type checker
forces).
Changes
Protocol (
crates/protocols/src/responses.rs):ResponseTool::Namespace { description: String, name: String, tools: Vec<NamespaceTool> }tagged#[serde(rename = "namespace")].NamespaceTool = Function(FunctionTool) | Custom(CustomTool)enum. ReusingResponseToolfor the inner element type would structurally allow recursiveNamespacenesting and hosted/built-in tools — both forbidden by spec.Integration tests (
crates/protocols/tests/responses.rs, 4 new):test_namespace_tool_with_function_round_trip— namespace with a singleFunctionelement.test_namespace_tool_with_custom_text_format_round_trip— namespace with aCustomtool usingTextformat.test_namespace_tool_with_custom_grammar_format_round_trip— namespace with aCustomtool usingGrammarformat (bothlarkandregex).test_namespace_tool_mixed_elements_round_trip— single namespace mixingFunctionandCustomelements.Forced-cascade router arms (compile-driven, no behavior beyond the pre-existing pattern):
response_tool_to_valueinmodel_gateway/src/routers/openai/responses/utils.rs— routeNamespacethroughserde_json::to_value, mirroring howCustom/ hosted tools already round-trip back to the client.tool_typesdebug mapping inmodel_gateway/src/routers/grpc/harmony/builder.rs— emit"namespace"to match the spec discriminator (same pattern as all sibling arms).No guardrails, no validation, no cross-file refactors. Blocks: unblocks T10 (
ToolSearchhosted/client tool which depends on the fullAnyToolunion includingNamespace).Test plan
cargo check -p openai-protocol --tests— clean.cargo test -p openai-protocol --test responses— 63 passed (4 new).cargo check -p smg --lib— clean after forced-cascade arms added.cargo fmt --all— clean.cargo clippy -p openai-protocol -p smg --lib --tests -- -D warnings— clean.Refs: .claude/_audit/responses-api-gap-audit.md §T9, .claude/_audit/openai-responses-api-spec.md L475
Summary by CodeRabbit
New Features
Bug Fixes
Tests