feat: codex integration#84
Conversation
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
|
awesome @haoshan98 let me know when this is ready for review |
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
ashwing
left a comment
There was a problem hiding this comment.
Took an early look since this is on the critical path for the tool-loop work in #83/#89 — flagging structure/scoping now while it's cheap to reshape, rather than after it's built out. cc @maralbahari
Big enough (and draft) that it's worth asking whether it wants to land as one PR or a few. From the outside it reads as ~three separable concerns: (a) the typed namespace model — ResponsesTool::Namespace + flatten_tools_for_upstream/restore_output_items; (b) the raw store=false proxy path; (c) model aliasing (resolve_model_alias + the proxy rewrite), which has no namespace dependency at all. Each is independently reviewable, and landing them in sequence would make this far more tractable than one 12k diff — aliasing especially looks like it could go first or separately with almost no coupling. Is there coupling between these that makes slicing harder than it looks?
| } | ||
|
|
||
| #[must_use] | ||
| pub fn flatten_raw_tools_for_upstream( |
There was a problem hiding this comment.
The raw path mirrors the typed one — same flatten/restore rule, but on serde_json::Value + SSE-line string rewriting instead of ResponsesTool (restore itself is a clean map lookup — that's fine; it's the outbound SSE-line surgery that's fragile). Two impls of one rule will drift — worth centralizing on one core with thin typed/raw adapters over it.
| pub const MODEL_VISIBLE_NAMESPACE_MEMBER_PREFIX: &str = "agentic_ns__"; | ||
|
|
||
| #[must_use] | ||
| pub fn model_visible_namespace_member_name(namespace: &str, member: &str) -> String { |
There was a problem hiding this comment.
agentic_ns__{namespace}__{member} joined on __ — the builder catches flattened-vs-top-level collisions but not two namespace+member pairs flattening to the same string (ns a__b+member c vs ns a+member b__c both → agentic_ns__a__b__c); record_flat_member just inserts, silent last-write-wins. MCP names use __ heavily, so not hypothetical. Worth detecting the flattened-name collision at build — even a warn — before this hardens.
There was a problem hiding this comment.
Solid catch, added a build-time warning for flattened-name collisions in record_flat_member with a test covering the a__b+c vs a+b__c case. Note the behaviour on collision is still last-write-wins. A proper fix needs a collision-free encoding, which I'd like to do as a follow-up.
…handling and fix per-line map rebuild Signed-off-by: maral <maralbahari.98@gmail.com>
agree we can separate out the support case on proxy path into separate PR. main focus of this PR to land in first for gateway support. however I am not sure for model aliasing would be needed for gateway @haoshan98 ? |
Will separate proxy changes so this one stays focused on typed/stateful gateway namespace support. Model aliasing is not strictly needed, just an optional Codex UX/routing convenience, can keep it later as well. |
Signed-off-by: haoshan98 <haoshanw@gmail.com>
|
@franciscojavierarceo @leseb Proxy-path namespace support will be split out and land in a follow-up PR; this PR keeps namespace handling on the executor path only. Ready for review. |
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
|
took another pass with the Rust/API-shape lens. i think the core implementation is in pretty good shape, but i’d revise a few things before merging:
I checked the proposed |
franciscojavierarceo
left a comment
There was a problem hiding this comment.
this is great!
I'm going to cut a follow up PR to handle my feedback so we can land this.
|
Thanks for folding in the feedback. Since it's already merged, quick status on where the two points landed:
I'll pick up the |
Extract the gateway tool loop's per-round decision into a LoopDecision enum + classify_round() classifier (executor/gateway.rs) and drive run_until_gateway_tools_complete by matching on it, replacing the inline if/if/else branches. Reworked onto the merged per-call ToolRegistry::dispatch() (vllm-project#82/vllm-project#85) rather than a parallel dispatch surface, and consolidating vllm-project#84's inline client-action path onto LoopDecision::RequiresClientAction. Behavior: - Exhausting the round budget while the model still requests tools now returns status "incomplete" + incomplete_details.reason instead of Err, preserving accumulated output and recording the final round's gateway calls with their outputs so a continuation is never fed a dangling tool call. - Per-call timeout (GATEWAY_TOOL_TIMEOUT 60s; Duration::ZERO opts out): a hung tool becomes an error output fed back to the model, not a whole-request failure. - Replaced the hard >8 calls/round cap with a .buffered(5) sliding concurrency window that drains arbitrary fan-out without failing. - A gateway-owned tool declared with no registered handler now yields an error output too, making the "never a whole-request failure" contract total. Signed-off-by: Ashwin Giridharan <girida@amazon.com>
Extract the gateway tool loop's per-round decision into a LoopDecision enum + classify_round() classifier (executor/gateway.rs) and drive run_until_gateway_tools_complete by matching on it, replacing the inline if/if/else branches. Reworked onto the merged per-call ToolRegistry::dispatch() (vllm-project#82/vllm-project#85) rather than a parallel dispatch surface, and consolidating vllm-project#84's inline client-action path onto LoopDecision::RequiresClientAction. Behavior: - Exhausting the round budget while the model still requests tools now returns status "incomplete" + incomplete_details.reason instead of Err, preserving accumulated output and recording the final round's gateway calls with their outputs so a continuation is never fed a dangling tool call. - Per-call timeout (GATEWAY_TOOL_TIMEOUT 60s; Duration::ZERO opts out): a hung tool becomes an error output fed back to the model, not a whole-request failure. - Replaced the hard >8 calls/round cap with a .buffered(5) sliding concurrency window that drains arbitrary fan-out without failing. - A gateway-owned tool declared with no registered handler now yields an error output too, making the "never a whole-request failure" contract total. Signed-off-by: Ashwin Giridharan <girida@amazon.com>
The framework shipped across vllm-project#80/vllm-project#82/vllm-project#84/vllm-project#85/vllm-project#91 (with vllm-project#83/vllm-project#89 in review) and diverged from the original proposal in several ways. Update the doc to the design-of-record: - Status: Proposal -> Accepted; add as-built preamble. - ToolHandler split into ToolHandler + GatewayExecutor (execute() only applies to gateway-owned types); Pin<Box<dyn Future>> not #[async_trait]; no discover()/event_prefix()/output_item_type() on the trait. - ToolType gains CodexNamespace (6 variants); ResponsesTool shown as the shipped #[non_exhaustive] 7-variant enum incl. namespace + Unknown catch-all and the web_search aliases. - ToolEntry carries handler: Option<Arc<dyn GatewayExecutor>>; dispatch is two layers (per-call registry.dispatch + multi-turn classify_round/ LoopDecision) with a diagram. - LoopDecision trimmed to 4 unit variants; mixed turns resolved by classify_round precedence, not ContinuePartial. - Add Implementation Status (PR mapping) and Future Work (layering ADR, GatewayAccumulator, per-type config, remaining handlers). - Mark Open Questions Q1-Q4 resolved; clarify collision handling is a hard error only for namespace-vs-top-level, warn-level last-write-wins otherwise. Rationale sections (Principles, Alternatives Considered) preserved. Signed-off-by: Ashwin Giridharan <girida@amazon.com>
## Summary Extracts the gateway tool loop's per-round decision into a `LoopDecision` enum + `classify_round()` classifier (in `executor/gateway.rs`), drives `run_until_gateway_tools_complete` by matching on it, and hardens the tool-execution path with a per-call timeout and bounded-concurrency fan-out. Reworked onto what's now on `main`: rather than a parallel dispatch mechanism, it **builds on the merged per-call `ToolRegistry::dispatch()` (#82/#85)** and layers multi-turn orchestration on top — the unification the MCP design doc (#82) calls for ("unified into `execute_loop()`"). Rebased onto the merged codex integration (#84) so its inline client-action path converges onto a single `LoopDecision::RequiresClientAction`. ## What changed **Loop control — `executor/gateway.rs` + `engine.rs`** - `LoopDecision` — `#[non_exhaustive]`: `Continue` / `Done` / `RequiresClientAction` / `Incomplete(String)` — and `classify_round(has_client_owned, gateway_results, round, max_rounds)`, one place that decides whether a turn loops, stops, hands back to the client, or exhausts the budget. Client-owned calls take precedence; the round cap only fires when gateway work remains. - `run_until_gateway_tools_complete` matches `classify_round(...)` instead of the previous inline `if/if/else` branches. Per-call dispatch stays on `registry.dispatch()`; SSE lifecycle emission is unchanged. - **Behavior change:** exhausting the round budget while the model still requests tools now returns `status: "incomplete"` + `incomplete_details.reason` instead of `Err`, matching the Responses API and preserving accumulated tool output. The final round's gateway calls and their outputs are recorded, so a continuation is never fed a dangling tool call. - `RequiresClientAction` covers both plain `function` and Codex `namespace` calls (`is_gateway_owned() == false`). In a **mixed turn** — a single response emitting both a gateway `web_search` and a client-owned Codex call — the gateway call is executed this turn and the client call handed back in the same `RequiresClientAction` round (no extra inference round-trip; the `web_search` result comes back already resolved). **Tool-execution hardening — `executor/gateway.rs`** - **Per-call timeout** (`GATEWAY_TOOL_TIMEOUT`, 60s; `Duration::ZERO` opts out). A tool exceeding the budget becomes an error output fed back to the model — one hung provider cannot stall the round. - **Removed the hard `>8 calls/round` cap** that returned an error. The `.buffered(MAX_CONCURRENT_GATEWAY_CALLS = 5)` sliding window already bounds outbound fan-out (admits the next call as one finishes); arbitrary fan-out drains without failing the request. Call count is bounded upstream by model output size. - A gateway-owned tool declared with **no registered handler** (server built without that executor) now yields an error output too, rather than failing the whole request — making the "never a whole-request failure" contract total. ## Test Plan - [x] `cargo test --workspace` — 325 pass, 0 fail (1 pre-existing vLLM cassette ignored) - [x] `cargo clippy --workspace --all-targets -- -D warnings` — clean - [x] `cargo fmt --check` — clean **Coverage:** - `gateway.rs` unit tests — all four `LoopDecision` arms + the final-round-no-work-is-`Done` edge; a per-call-timeout test (1ms budget vs a 50ms tool → error output fed back); a missing-handler test (gateway-owned tool with no executor → error output). - `tests/dispatch_loop_cassette_test.rs` (new) — 3 integration tests driving recorded OpenAI Responses wire bodies through the loop: single client-owned call terminates in one round, parallel calls preserved, mixed gateway + client-owned. - `tests/web_search_tool_test.rs` — round cap asserts the `status: "incomplete"` contract (blocking **and** streaming); a continuation test proves the persisted incomplete turn pairs every gateway call with its output (no dangling call); a 9-call fan-out test proves all execute despite the concurrency window of 5; a mixed-turn test covers `web_search` (gateway) + a Codex namespace call (client) resolving in one `RequiresClientAction` round, with the namespace call restored to `{namespace, name}` and the continuation carrying the persisted `web_search` output. ## Notes **Dispatch model:** no separate dispatch surface — uses the merged `ToolRegistry::dispatch()` for per-call resolution and owns only the multi-turn loop. `LoopDecision` is `pub(super)`, sufficient for #84's consolidated path and PR C (both under `executor`). **Follow-ups (non-blocking, out of scope here):** the per-call timeout and concurrency window are file-private consts — promoting them to per-tool-type config (as varied gateway providers land via #82) is an additive change on the existing `execute_gateway_call_with_timeout(timeout)` seam. There is no outer request-level deadline on the executor HTTP client yet; the per-call timeout bounds a single call, not total request time. AI assistance was used in drafting this PR. --------- Signed-off-by: Ashwin Giridharan <girida@amazon.com> Co-authored-by: Francisco Javier Arceo <arceofrancisco@gmail.com>
Typed Codex Namespace Support For Responses Gateway
Summary
This PR adds typed, stateful Codex namespace tool support to the Responses gateway path.
Codex can send
type: "namespace"tool declarations, while vLLM expects ordinary function-tool declarations. Thegateway now keeps the Codex-facing tool shape at the request/storage boundary, flattens namespace members only when
building the upstream request, and restores upstream flat function calls back to Codex's public
{ namespace, name }shape in final and streaming responses.
Main changes:
ResponsesTool::Namespaceand Codex namespace member types.CodexNamespaceHandlerandNamespaceMapfor request-scoped flatten/restore.RequestPayload::to_upstream_request().tool_choicevalues to the same flattened names sent upstream.SKIP_LLM_READY_CHECKandscripts/codex-start-gateway.shfor manual demo runs.The intended runtime path is:
Gateway Flow
Codex can declare a namespace tool:
{ "type": "namespace", "name": "mcp__agentic_fixture", "tools": [ { "type": "function", "name": "add_numbers" } ] }The request and storage layers keep that public shape as
ResponsesTool::Namespace.When the gateway builds the upstream request,
RequestPayload::to_upstream_request()callsCodexNamespaceHandler::resolve_namespace_members()and turns namespace members into model-visible function names:The upstream request receives only
FunctionTooldeclarations. The namespace container itself is not sent to vLLM.When the model calls the flattened function, the gateway uses the request's
NamespaceMapto restore the Codex-facingshape:
{ "type": "function_call", "namespace": "mcp__agentic_fixture", "name": "add_numbers" }The same mapping is used for:
tool_choiceTool Registry Integration
Namespace support is wired through the existing tool abstractions rather than a separate request-side tool model.
ResponsesToolremains the public request/storage enum.CodexNamespaceHandler::resolve_namespace_members()performs the pure request-scoped rename pass.ResponsesTool::to_function_tools()converts the already-renamed typed tools into upstreamFunctionToolvalues.ToolRegistry::build_with_handlers()resolves namespace members before registering model-visible tool names.ToolRegistry::restore_final_payload_output()restores completed response payloads.ToolRegistry::restore_stream_event_value()restores streaming event payloads.This keeps the special Codex namespace behavior in one handler while preserving the registry/handler convention used by
the rest of the gateway.
Stateful Continuation
Stateful Responses execution stores the effective request metadata needed by later turns:
Follow-up requests using
previous_response_idorconversation_idrehydrate prior context and inherit effective toolmetadata unless the client explicitly overrides it.
tool_choiceis represented asOption<ToolChoice>onRequestPayload, so rehydration can distinguish a missingchoice from an explicitly provided choice. Missing request-side tool choice resolves to upstream
auto; explicit valuesremain explicit.
Instructions stay as top-level Responses
instructions. The gateway does not synthesize an extra system message.Proxy And Routing Scope
This PR intentionally keeps raw proxy behavior small and transparent.
HTTP
/v1/responsescan still use the proxy path for stateless, function-only requests that do not need gatewayexecution:
store = falseprevious_response_idconversation_idRequests that need storage, continuation, WebSocket handling, gateway-owned tools, or Codex namespace tools go through
the typed executor path.
Out of scope for this PR:
Codex still owns client-side tool execution.
Collision Handling
The model-visible namespace member format is:
The
agentic_ns__prefix avoids provider-reserved prefixes such asmcp__and marks generated names as gateway-ownedflattened namespace members.
The namespace map builder avoids flattening a namespace member over an existing top-level tool name. It also warns when
two namespace/member pairs flatten to the same model-visible string, because MCP-style names often contain
__and canotherwise become ambiguous.
Cassettes And Tests
This PR adds Codex cassette coverage under
crates/agentic-core/tests/cassettes/codex/for:It also adds recorder/support scripts for regenerating those cassettes.
Change Size Analytics
Most of the PR size is recorded cassette data: YAML cassettes account for 7,608 added lines, about 64% of total
additions. The production and benchmark code portion is 1,789 added lines and 135 deleted lines.
Bucket rules used for this snapshot:
*.yaml/*.ymlfiles are counted as YAML cassettes.*.jsoncassette support files are counted as JSON fixtures.scripts/*,*.sh, and*.pyfiles are counted as scripts.tests/are counted as test code unless they match a cassette/fixture/script rule above.docs/*files are counted as docs.Test Plan
Important coverage:
agentic_ns__...upstream function declarations.tool_choicevalues are rewritten before upstream serialization.tool_choicevalues are not accidentally rebound to namespace members.{ namespace, name }calls.