Repository navigation
[Bugfix][Responses API] Render namespace tool declarations instead of dropping them - #39174
hawkli-1994 wants to merge 1 commit into
Conversation
ce0db46 to
3bfba75
Compare
|
Hi @JustinTong0323 — requesting a review when you have a moment, and the To make coordination against the neighboring PRs easy, the scope here is deliberately narrow — strictly the silently-dropped-declaration bug for
Verification so far: 81 unit cases green across the four responses suites; each new guard was verified red against pre-fix serving code before being trusted; all applicable pre-commit hooks pass (enumerated in the checklist). I'm glad to run a live-server / Codex CLI conformance pass and post the record in the same format as your #38690 verification — pointers to the preferred harness welcome. Will rebase promptly whenever main moves. |
|
Live repro record (stock release, pre-fix) — the silent drop on a real server, matching the Motivation section: Setup: sglang 0.5.19 (stock release), Case 1 — custom tool declaration silently dropped (
So the request "succeeds" while the tool loop is dead — the client can never parse this into a call item. This is the exact Codex-killing failure mode from #35216. Case 2 — namespace declaration accepted, inner tools never reach the model (
(Side observation: 0.5.19's request schema requires inner namespace members to carry Next: same script in fix mode against a server built from this branch, record to follow in the same format. |
|
Live fix-mode verification record — same release, same wire behavior, only this PR's files changed. Setup — an internal 1× L20 (48GB) host, sglang 0.5.19 (stock release wheel) with this PR's four changed files overlaid verbatim ( Results — 4/4 PASS
Regression check — a plain Alongside the pre-fix repro on stock 0.5.19 (previous comment: declaration 200 but no call ever emitted; namespace members never reachable), this pins the fix end to end on a real model: declaration reaches the model through the shim, calls come back on the right wire items with the right fields, replay no longer breaks the loop, streaming sends the custom-tool event stream, and plain function tools are unaffected. Happy to re-run the same suite against any model/parser combo the reviewers prefer. |
|
Rebased onto main after #38690 landed: the head branch is now |
3bfba75 to
982a34e
Compare
|
Live verification record against the rebased head ( Results — 4/4 PASS on the conformance script, now doubly meaningful:
Namespace streaming additionally verified live: a So on a real model: main's custom-tool support is intact under this diff, and the namespace half now behaves identically on the wire. |
… dropping them
A `namespace` declaration groups inner function schemas (Codex sends its
multi-agent tool families this way; ProviderCapabilities::namespace_tools
defaults to true). The protocol accepts the declaration but nothing
executes it: the inner functions never reach the chat tools the model sees,
so the model never calls them, and a replayed call carrying a `namespace`
field loses its qualification.
Members now flatten to `{namespace}.{name}` chat function tools (inner
description and strict flags preserved; the namespace description fills
members that omit one), and calls translate back on every output path:
- non-streaming and the required-JSON fallback split the qualified name
into `name` plus a `namespace` field on the item
- streaming `output_item.added/done` carry the split pair
- replayed namespaced calls re-qualify for the chat template; only
declared namespace prefixes split, so a plain function whose name
contains a dot is never reinterpreted
- the harmony developer message renders the flattened members, and
harmony output parsing splits qualified recipients the same way
Namespaced items travel through widened pydantic unions
(`ResponseNamespacedFunctionToolCall` plus matching event subclasses);
without the widened arm leading, the SDK's `ResponseFunctionToolCall` arm
serializes the subclass and silently drops `namespace`.
`tool_choice="required"` accepts namespace declarations, and a `namespace`
field on a named choice re-qualifies it to the flattened name instead of
degrading to "auto".
Custom tools (the other half of sgl-project#35216) landed on main via sgl-project#38690; this
PR builds on that work and shares its adapters module.
Co-Authored-By: Claude Code <noreply@anthropic.com>
982a34e to
771843d
Compare
|
Gentle follow-up on this one, @JustinTong0323 — happy to make any changes if the approach needs adjusting. Where it stands since the last note: rebased cleanly onto current main (single commit), the CI Would appreciate the |
Motivation
A
namespacetool declaration groups inner function schemas — Codex sends its multi-agent tool families exactly this way (ProviderCapabilities::namespace_toolsdefaults totrue, so custom providers receive them un-flattened)./v1/responsesaccepts the declaration but nothing executes it:customtools had before [Feat][Responses API] Support custom tools, encrypted reasoning replay, developer tier and model validation #38690.function_callcarrying anamespacefield loses its qualification, breaking stateless tool loops.This is the namespace half of #35216. The custom-tool half landed on main via #38690; this PR builds directly on that work (same
responses_adapters.pymodule, same patterns) so the two read as one coherent story. Scope relative to #38359 (also namespace-only, open): same wire contract, plus harmony-path support, the streamingadded/donesplit, and the pydantic union serialization that otherwise silently dropsnamespace.Modifications
All changes are stateless HTTP-serving-layer translation; no tokenizer, template, engine, or scheduler changes. Namespace helpers join
responses_adapters.pynext to the custom-tool ones:{namespace}.{name}chat function (inner schema/description/strict preserved; the namespace description fills members that omit one). Shared by the chat path, the harmony developer message, and (vianamespace_members_to_chat_tools) future callers.name+namespaceon the item: non-streaming, the required-JSON fallback, and the streamingoutput_item.added/.doneevents.function_callwithname+namespacere-qualifies to the flattened name for the chat template, and the harmony recipient likewise. Only declared namespace prefixes split; an exact declaration outranks the split, so a plain function whose name contains a dot is never reinterpreted.ResponseNamespacedFunctionToolCall, matchingoutput_item.added/doneevent subclasses, and theResponsesResponse.outputunion). Without the widened arm leading, the SDK'sResponseFunctionToolCallarm serializes the subclass instance and silently dropsnamespace— a trap any subclass-based approach hits."required"accepts namespace declarations, and anamespacefield on a named choice re-qualifies it to the flattened name instead of degrading to"auto".Accuracy Tests
No kernel or model-forward changes; wire behavior only. Namespace cases join the existing custom-tools suite (
test/registered/unit/entrypoints/openai/test_responses_custom_tools.py, registered inbase-a-test-cpu):All four green on this branch. Regression verification: with the source changes stashed (pre-fix main), the suite goes red at import (
cannot import name 'split_namespaced_call'); each namespace guard targets a distinct failure mode (declaration drop, split precedence, replay re-qualification, output emission, streaming wire events, union serialization, harmony rendering).Live verification against real servers is recorded in the comments below: pre-fix stock sglang 0.5.19 (declaration accepted, inner tools never reachable), and the same release with this PR's files overlaid (qualified calls split correctly on a live Qwen3-1.7B). The overlay there predates this rebase onto #38690; happy to re-run the record against this exact head on request.
Speed Tests and Profiling
Not applicable — no inference-path changes. Runtime cost is one pass over
request.toolsper request and a set lookup per parsed call.Checklist
F401/F821/UP037), ruff-format, isort, and codespell pass on the changed files;git diff --checkclean.base-a-test-cpusuite file; every guard verified red against pre-fix main.Note for reviewers: the head branch moved from
fix/responses-custom-namespace-toolstofix/responses-namespace-tools(rebased onto main after #38690 landed) — the comment history below, including the live verification records, refers to that earlier iteration which covered custom + namespace together.🤖 Generated with Claude Code
CI States
Latest PR Test (Base): ❌ Run #34702424926
Latest PR Test (Extra): ❌ Run #34702424700
Latest PR Test (AMD ROCm 10): ❌ Run #34702424829