refactor: drive gateway tool loop with LoopDecision (PR B)

[ashwing]

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

• cargo test --workspace — 325 pass, 0 fail (1 pre-existing vLLM cassette ignored)
• cargo clippy --workspace --all-targets -- -D warnings — clean
• 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.

[ashwing]

cc @maralbahari @franciscojavierarceo — PR B from the tool framework design (#67). Builds on the types/registry from #80. The main open question is the dispatch model conflict with #82 (raised there already).

[maralbahari]

@ashwing Thank you for the PR. we have now a working web search tool support in main. I think instead of mock implementation would be best to refactor the current agentic-core/executor/gateway.rs for execution_loop and LoopDecision and dispatch_tool based on web_search to based on your PR design here.

[ashwing]

Agreed, that's the right move. Let me fold it into what's on main rather than the other way around.

The way I see it there are two separate layers here, and only one of them is contested:

• Per-call dispatch — registry.dispatch(call) with the handler on ToolEntry (from feat: add web search gateway tool #85) is cleaner than the gateway_owned() + external executor map I had in dispatch_tools. I'll drop mine and build on dispatch().
• The multi-turn loop — right now that lives inline in engine.rs (for _ in 0..MAX_GATEWAY_TOOL_ROUNDS). That's the part refactor: drive gateway tool loop with LoopDecision (PR B) #83 pulls into a tested execute_loop + LoopDecision, which is also what your MCP doc calls for at the end ("unified into execute_loop()"). So I'll lift the loop out of engine.rs into execute_loop and have it drive the gateway.rs helpers you already have — dispatch, public_output_items, the SSE emitters stay put.

A few things I'd keep from #83 because they're behavior changes, not just refactor:

• persistence triggers (store / previous_response_id / conversation_id) cleared for the internal turns and restored on the final payload only
• cap-exceeded returns status: "incomplete" rather than the current hard error — that's the Responses-API-correct semantics, worth a look
• LoopDecision is #[non_exhaustive], so new variants won't break existing arms

A couple of calls I'm making:

1. Loop home — it goes in gateway.rs next to the execution helpers, not a separate agentic_loop.rs. The loop and the dispatch/SSE helpers are one unit; splitting them just adds a hop for no benefit.
2. RequiresClientAction — adding it to LoopDecision here so there's a single enum, and I'll coordinate with @haoshan98 to drop the duplicate from feat: codex integration #84. One loop vocabulary beats parallel sequencing, and feat: codex integration #84's still in draft so now's the cheap time to converge.

[ashwing]

@maralbahari — reworked the loop around execute_loop/LoopDecision. It builds on the merged registry.dispatch() rather than a separate mechanism; gateway.rs owns classify_round + the LoopDecision match, engine.rs just runs the rounds.

Two behavior changes worth a look: the round cap now returns status:"incomplete" (not an error), preserving accumulated output, and I added a per-call tool timeout + dropped the hard >8-call fanout cap in favor of the bounded buffered window. RequiresClientAction is in the enum as the seam #84's client-owned path can converge onto.

283 tests, CI green.

[maralbahari]

A couple of calls I'm making:

1. Loop home — it goes in gateway.rs next to the execution helpers, not a separate agentic_loop.rs. The loop and the dispatch/SSE helpers are one unit; splitting them just adds a hop for no benefit.
2. RequiresClientAction — adding it to LoopDecision here so there's a single enum, and I'll coordinate with @haoshan98 to drop the duplicate from feat: codex integration #84. One loop vocabulary beats parallel sequencing, and feat: codex integration #84's still in draft so now's the cheap time to converge.

@ashwing I checked #84 the dispatch.rs file in this PR would be removed actually cause it is not really utilized anywhere as the main feature is to enable codex namespace tool to work and be executed on codex not on agentic-api. so in this PR you would be handling those paths to handle gateway vs client dispatch, execution and perhaps the event emits after the each execution either by client or gateway we would need to emit the result events. need to handle those extensible in such way that any tool would just plug in their output result type and the emit event result would take those. would require some thoughts on that end too.

[ashwing]

Ordering works — #84 → #83 → #89. #84's loop is still the old inline one, so I'll fold the consolidation into #83: RequiresClientAction becomes the actual client-owned path (pulls in requires_client_action instead of a separate branch). One loop for both — gateway exec and handing client calls back.

On the pluggable per-tool output/emit — how firm is the shape from codex's side? gateway_public_output is just a match tool_type right now (web_search, + MCP when #89 lands). Two arms, both readable. My instinct is add codex as the third arm, then pull the extension point out once the shape's obvious from three real cases — abstracting from two usually guesses wrong. But if #84's already telling you what that interface needs, that's the third case in hand and I'd defer to that. What are you seeing?

I'll review #84 so it can go first — that's the long pole, and it's what lets #83 consolidate on real client behavior instead of a stub.

[maralbahari]

On the pluggable per-tool output/emit — how firm is the shape from codex's side? gateway_public_output is just a match tool_type right now (web_search, + MCP when #89 lands). Two arms, both readable. My instinct is add codex as the third arm, then pull the extension point out once the shape's obvious from three real cases — abstracting from two usually guesses wrong. But if #84's already telling you what that interface needs, that's the third case in hand and I'd defer to that. What are you seeing?

@ashwing I dont have clear picture now I think once we have both MCP, and codex namespace we can identify the common path and build from there. but one thing I foresee is we need to make the follow similar pattern as ResponseAccumulator how it uses the normalized SSE for gateway public streaming shape.

Gateway + Response Accumulator Flow

Raw Upstream EventFrame
        |
        v
Raw ResponseAccumulator
  - reads the model inference stream
  - accumulates messages/reasoning/function-call args
  - turns streamed deltas into complete FunctionToolCall state
  - emits accumulator deltas such as:
      FunctionCallStarted
      FunctionCallArgumentsDelta
      FunctionCallDone
      OutputItemDone
      ResponseCompleted
        |
        v
GatewayAccumulator
  - consumes raw accumulator deltas
  - classifies completed function calls with ToolRegistry
  - passes client-owned calls through
  - treats CodexNamespace calls as client-owned
  - hides gateway-owned function calls
  - emits synthetic web_search_call / mcp_tool_call frames
  - spawns/polls gateway executions
        |
        v
Visible EventFrames
  - normal message/reasoning frames
  - client-owned function_call frames
  - synthetic web_search_call / mcp_tool_call frames
        |
        v
Public ResponseAccumulator
  - accumulates only what the client is allowed to see
  - accumulates messages
  - accumulates client function calls
  - accumulates web_search_call / mcp_tool_call from output_item.done
        |
        v
ResponsePayload / Stream Output

[ashwing]

@maralbahari Yeah, that settles the extension-point question — hold it until MCP and codex are both in, then pull the common path from three real cases. No guessing from two.

The GatewayAccumulator layering is the right shape, and it lines up with something already bugging me in this PR: the "hide gateway-owned calls, emit the synthetic web_search_call frame" logic lives in two places right now — public_output_items for blocking, and emit_gateway_start/completed_events for streaming. Both re-derive the same classification off the registry. Your pipeline collapses that into one stage that does it once and both paths consume — which is exactly where the MCP/codex synthetic frames would slot in too.

I'd keep that out of this PR though — it's a streaming-path refactor with its own surface, and #83 is already the loop consolidation. Once #89 (MCP) lands we'll have the two gateway frame types plus codex's client-owned case in hand, and the accumulator seam becomes concrete instead of a third guess. Happy to take that as the follow-up.

PR's rebased onto current main and green now, if you get a chance to look at the loop itself.

[maralbahari]

I think the unit test here are not really necessary. I think with an integration test should be the best scenario to verify the loop. like we have cassettes for both web_search and codexnamespace could combine the two like one response/conversation turn from web_search cassettes another turn from codexnamespace cassettes. this would be good scenario for the integration test.

[ashwing]

Two things here — the integration test I'm on board with, the unit tests I'd keep.

Integration test: agreed, good use of the cassettes we've got. I'll add one that runs a web_search turn then a codexnamespace turn (codex-gateway-http-namespace-tool-*) through the loop in one conversation — gateway turn resolves server-side, namespace turn hands back RequiresClientAction with the call restored. web_search_tool_test.rs already has this shape inline (execute_runs_gateway_web_search_and_hands_back_codex_namespace_in_one_round), so I'll move it onto the cassette rather than keep both.

On the classify_round units though — I'd keep them. It's a pure function and they pin the whole decision table cheaply: the off-by-one at round + 1 >= max_rounds and "final round with no gateway work is still Done." Hitting those through cassettes means recording 10-round conversations just to reach the boundary. Feels cleaner to keep the table pinned at the unit level and let the integration test prove the wiring. Sound right?

[ashwing]

Done — added openai_gateway_web_search_then_codex_namespace_across_turns in dispatch_loop_cassette_test.rs: a gateway web_search turn then a namespace turn in one conversation. Round 0 executes web_search server-side and continues; round 1 restores the flat agentic_ns__…__add_numbers call to {namespace, name} and hands back RequiresClientAction. Moved the old inline single-turn test out since this covers it and the multi-round transition too. Kept the classify_round units for the boundary cases.

[maralbahari]

great. then can remove this unit test here

[maralbahari]

I think there is no need to create this cassettes you can simply load https://github.com/vllm-project/agentic-api/blob/main/crates/agentic-core/tests/cassettes/codex/codex-gateway-http-namespace-tool-Qwen-Qwen3.6-35B-A3B-streaming.yaml
And
https://github.com/vllm-project/agentic-api/blob/main/crates/agentic-core/tests/cassettes/web_search/gpt_oss_web_search_streaming.yaml
for another turn
then test the loop.

[ashwing]

@maralbahari dug into this and the two cassettes won't chain through one loop run — they're recorded in different formats. The web_search one (openai_responses_tool_calls_3turn_streaming turn 2) is typed incremental SSE (response.output_item.added … response.completed), which the streaming accumulator parses via from_sse_lines. The codex namespace one is a single completed-response JSON blob on one data: line — the existing codex tests read it with from_json, not the streaming path. stream_upstream is fixed for the whole run, so a streaming loop can't consume the codex blob (I tried — the namespace turn drops out entirely) and a blocking loop can't consume the web_search SSE.

That mismatch is why I went with the composed cassette — it's real recorded shapes for both turns in one compatible format, and it does drive the actual paths: gateway web_search executes → Continue, then the flat agentic_ns__… call restores to {namespace, name} → RequiresClientAction. The only synthetic part is the YAML wrapper, not the traffic.

Happy to reshape the codex blob into typed SSE if you'd rather it come from a recording, but that's effectively re-recording for the same coverage. I'd keep the composed cassette unless you want to go that route.