refactor(responses): split load/prepare for canonical agent-loop input

[zhaowenzi]

Parent: #1316
Closes #1317

Description

Problem

Responses-side agentic control flow in SMG is spread across layers:

• The OpenAI Responses router (streaming + non-streaming), the gRPC regular Responses router, and the gRPC harmony Responses router each decide loop entry outside the loop — branching on "does this request carry MCP tools?" before calling surface-specific tool loops.
• Each surface re-implements history loading, upstream request rebuilding, final response assembly, persistence, and streaming completion handling.
• Three representations bleed together without explicit boundaries: the client-facing ResponsesRequest / ResponsesResponse, the upstream model payload, and the stored response chain used by previous_response_id.
• As a result, behaviors like approval continuation tend to be added as new loop-external patch points instead of extensions of one explicit state machine. The mismatch also causes observable parity drift against OpenAI — e.g. previous_response_id replay currently drops mcp_list_tools / mcp_call items (they fail to deserialize as input) and downstream MCP calls get re-executed.

Solution

Introduce one shared agent loop used by every Responses surface. The loop decides the next action from state (CallLlm / ExecuteTools / InterruptForApproval / Finish). Every request enters the loop, even one with no MCP tools — it simply never produces an ExecuteTools action.

The plan formalizes three representation boundaries — the client-facing ResponsesRequest / ResponsesResponse, the canonical loop transcript, and the provider-specific upstream payload — and a surface-adapter contract covering history preparation, upstream request construction, turn ingestion, and final / interrupt / streaming rendering. MCP session state, mcp_list_tools emission dedupe, pending tool batches, approval interrupts, and max_tool_calls accounting all live on the loop state rather than being rediscovered at each patch point.

This design has already been implemented end-to-end once as a working prototype, with contract-level OpenAI parity checks for non-streaming and streaming MCP flows including approval interrupts. The expected outcome of the staged series below is to re-land that proven design on main in small, reviewable pieces without regressing anything along the way.

Staged PRs:

• PR1 (this PR) — Canonical input preparation for OpenAI Responses. Split history acquisition from normalization; accept input-side MCP trace items; upstream transcript is always LLM-consumable.
• PR2 — OpenAI non-streaming agent loop. Move to the NextAction driver; previous_response_id stops changing loop behavior after preparation; continuation is a loop transition, not a side path.
• PR3 — OpenAI streaming agent loop + stream sink. Same driver as non-streaming; response.completed assembled from loop state; sibling buffering for mixed gateway + user function calls.
• PR4 — OpenAI streaming approval parity. Streaming interrupt emits mcp_approval_request → output_item.done → response.completed → [DONE]; approved continuation obeys the same budget/error behavior as non-streaming.
• PR5 — Shared MCP presentation + visibility boundary. Centralize builtin ResponseFormat presentation and internal/hidden MCP filtering across streaming + non-streaming.
• PR6 — Extract routers/common/agent_loop. Move the OpenAI-proven abstractions behind a shared adapter trait.
• PR7 — Harmony adapter migration.
• PR8 — Regular (Chat-adapter) adapter migration.
• PR9 — Approval continuation generalization across surfaces.

Changes

Scope of this PR: OpenAI Responses only; history loading + input normalization only. Out of scope (deferred to later PRs): NextAction loop driver, streaming loop rewrite, new approval workflow behavior.

• Protocol (crates/protocols/src/responses.rs): add McpListTools and McpCall as ResponseInputOutputItem variants. Structural validation only — continuation-specific semantics (e.g. pairing an approval response with its originating request) are left to the loop-entry layer in PR2.
• model_gateway/src/routers/openai/responses/history.rs:
  • load_input_history now does source acquisition only — it fetches previous_response_id / conversation history and appends client input; no label extraction, no normalization.
  • New prepare_agent_loop_input is the single transcript-normalization boundary. It expands mcp_call into function_call + function_call_output so the upstream model sees a replayable tool execution, strips mcp_list_tools from the upstream transcript while recording its server_label for dedupe, and passes approval items through unchanged so the existing approval continuation flow (merged in feat(responses): interrupt approval-required MCP tool calls #1174) keeps working.
• model_gateway/src/routers/openai/responses/route.rs: calls the new preparation step between load_input_history and upstream request build; populates ResponsesPayloadState.existing_mcp_list_tools_labels from the preparation output.
• Touch-ups: exhaustive matchers in grpc/harmony/builder.rs, grpc/regular/responses/conversions.rs, and model_gateway/benches/routing_allocation_bench.rs include the two new enum variants (no behavior change).
• Updated test_previous_response_id_does_not_repeat_mcp_list_tools_for_existing_binding in tests/api/responses_api_test.rs: previously asserted the pre-normalization behavior where replay dropped MCP items and the mock had to re-execute the tool; now asserts the invariant — no repeated mcp_list_tools on a previous_response_id continuation for an existing binding, and a final message is still produced.

Test Plan

Automated — all green:

cargo test -p openai-protocol --tests                                   # 43 pass (+2 new)
cargo test --lib --package smg routers::openai::responses::             # 24 pass (+5 new)
cargo test --test api_tests -- responses                                # 33 pass
pre-commit run --all-files                                              # 17 hooks green

New tests:

• crates/protocols/tests/responses.rs::response_input_accepts_mcp_trace_items — protocol accepts mcp_list_tools and mcp_call as input items.
• crates/protocols/tests/responses.rs::mcp_call_input_omits_optional_fields_when_unset — approval_request_id / error omitted on serialize when None.
• openai::responses::history::tests::prepare_agent_loop_input_passes_text_through — text input identity.
• …::prepare_agent_loop_input_expands_mcp_call_to_function_pair — mcp_call → paired function_call + function_call_output with matching call_id.
• …::prepare_agent_loop_input_reuses_approval_request_id_for_call_id — resumed call derives its call_id from the originating mcpr_*.
• …::prepare_agent_loop_input_collects_list_tools_labels — mcp_list_tools stripped from upstream, server_labels deduped in first-seen order.
• …::prepare_agent_loop_input_preserves_approval_items_in_upstream — approval items pass through so existing continuation keeps working.

Manual OpenAI parity — same style as PR #1174's test plan. Launched local smg in IGW mode and registered the upstream OpenAI worker:

cargo run --bin smg -- --enable-igw --port 9999

curl -X POST http://127.0.0.1:9999/workers \
  -H 'Content-Type: application/json' \
  -d '{"url":"https://api.openai.com","api_key":"<OPENAI_API_KEY>","runtime":"external","disable_health_check":true}'

Five scenarios exercised end-to-end against real OpenAI, all behaving as expected:

1. Plain non-MCP request (gpt-5.4, text input) — completes with a single message.
2. MCP request (deepwiki, require_approval: never) — output order: mcp_list_tools → mcp_call → (optional second mcp_call) → message.
3. Stitched MCP input — the headline PR1 case. input carries message + mcp_list_tools + mcp_call (with a canned tool result) + assistant message + new user message. Upstream sees the tool result normalized into function_call + function_call_output and answers directly without re-executing the tool; response output is a single message containing the summarized answer. Without this PR the stitched MCP items fail to deserialize on the input side and the tool gets re-executed.
4. previous_response_id follow-up (after a stored MCP turn) — response has no repeated mcp_list_tools, no mcp_call, just a final message that reuses the tool result from the stored transcript.
5. Mixed approval (deepwiki: never + openai-developer-docs: always) — output: two mcp_list_tools, deepwiki mcp_call, terminates at mcp_approval_request exactly as PR feat(responses): interrupt approval-required MCP tool calls #1174 specified. Confirms the existing approval interrupt path is untouched.

Checklist

• cargo +nightly fmt passes
• cargo clippy --all-targets --all-features -- -D warnings passes
• (Optional) Documentation updated — the shared-loop design doc will land on main with the router-common extraction PR.
• (Optional) Please join us on Slack #sig-smg to discuss, review, and merge PRs

Summary by CodeRabbit

• New Features

  • Capture MCP tool listings and tool-call results in conversation transcripts.

• Improvements

  • Normalize MCP trace items before upstream routing: listings are stripped from prompt text and calls are expanded into correlated tool-call + output pairs.
  • MCP trace items are treated as non-text for routing and omitted from generated chat/harmony messages.
  • Preserve normalized upstream input for tool loops and resume flows.

• Tests

  • Added serialization, normalization, routing, and resume behavior tests for MCP traces.

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds McpListTools and McpCall response input variants and threads a normalized upstream_input through OpenAI routing, history preparation, converters, and the tool-loop; McpListTools is stripped (labels deduped) and McpCall expands into function call + output (error folded into output).

Changes

Cohort / File(s)	Summary
Protocol & Tests crates/protocols/src/responses.rs, crates/protocols/tests/responses.rs	Add ResponseInputOutputItem::McpListTools and ::McpCall variants with serde attrs; validation and text-extraction treat them as non-text; add round-trip and optional-field tests.
Routing Text Extraction model_gateway/benches/routing_allocation_bench.rs	Treat new MCP variants as non-text (map to None) in old extraction path.
gRPC / Conversions model_gateway/src/routers/grpc/harmony/builder.rs, model_gateway/src/routers/grpc/regular/responses/conversions.rs	Parser/converters updated to return/skip McpListTools/McpCall (warnings logged); Harmony parser now Result<Option<HarmonyMessage>, String> and skips MCP trace items.
OpenAI History & Normalization model_gateway/src/routers/openai/responses/history.rs, model_gateway/src/routers/openai/responses/route.rs	Introduce prepare_agent_loop_input and PreparedAgentLoopInput; load_input_history becomes source-only; prepare_agent_loop_input strips McpListTools (dedupe labels), expands McpCall → FunctionToolCall + FunctionCallOutput (error → output; call_id derived from approval_request_id or id suffix); route replaces request input with prepared upstream_input.
Context / Payload Propagation model_gateway/src/routers/openai/context.rs, model_gateway/src/routers/openai/responses/route.rs, model_gateway/src/routers/openai/responses/non_streaming.rs, model_gateway/src/routers/openai/responses/streaming.rs, model_gateway/src/routers/openai/mcp/tool_loop.rs	Thread upstream_input: ResponseInput through ResponsesPayloadState, OwnedStreamingContext, and ToolLoopExecutionContext; tool-loop initialization uses prepared upstream_input snapshot.
Tests & Integration model_gateway/tests/api/responses_api_test.rs, model_gateway/tests/routing/test_openai_routing.rs, router unit tests	Update tests to assert MCP items are not re-emitted on resumed turns, extend routing/storage assertions, and add unit tests for MCP-item stripping/expansion and converters skipping MCP items.

Sequence Diagram(s)

sequenceDiagram
    participant Client as "Client"
    participant Route as "route_responses"
    participant Load as "load_input_history"
    participant Prepare as "prepare_agent_loop_input"
    participant Converter as "converters / grpc builders"
    participant ToolLoop as "Tool Loop / Upstream"

    Client->>Route: POST ResponsesRequest(input)
    Route->>Load: load_input_history(previous_response_id / conversation)
    Load-->>Route: LoadedInputHistory
    Route->>Prepare: prepare_agent_loop_input(history + client input)
    Prepare->>Prepare: strip McpListTools (collect labels)
    Prepare->>Prepare: expand McpCall -> FunctionToolCall + FunctionCallOutput
    Prepare-->>Route: PreparedAgentLoopInput(upstream_input, labels)
    Route->>Converter: convert / build messages (skip MCP items)
    Converter-->>ToolLoop: push normalized messages
    ToolLoop-->>Route: tool loop results / final response
    Route-->>Client: Response

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• Unified agent loop for Responses API: design and implementation plan #1316 — Implements the canonical prepare/load split and MCP normalization objectives matched by these changes.
• [UAL-PR-02] OpenAI non-streaming agent loop #1318 — Overlaps on OpenAI agent-loop normalization and MCP-item handling implemented here.
• [UAL-PR-03] OpenAI streaming agent loop and stream sink #1319 — Related to streaming agent-loop and MCP-item replay/handling updated in this PR.

Possibly related PRs

• refactor(gateway): extract history loading and storage queries from router.rs #732 — Prior OpenAI responses history refactor this change builds upon.
• feat(protocols): implement I3 compaction input+output item #1302 — Adds similar ResponseInputOutputItem variants and updates conversion sites; strong code-level overlap.
• fix(mcp): avoid relisting tools on resumed responses #1060 — Addresses MCP list-tools/call handling in OpenAI responses; closely related.

Suggested reviewers

• CatherineSue
• key4ng
• slin1237

Poem

🐰 I hopped through traces, tidy and small,
List-tools hidden, calls unfolded for all.
I pair each call with its output bright,
Upstream now neat for the tool loop's flight. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main structural change: splitting load/prepare phases for canonical agent-loop input normalization in the responses router.
Linked Issues check	✅ Passed	The PR implements all acceptance criteria from #1317: load_input_history is source-acquisition only, prepare_agent_loop_input handles normalization with mcp_call expansion and mcp_list_tools stripping, protocol variants added, and all required test coverage included.
Out of Scope Changes check	✅ Passed	All changes are directly related to the stated objectives. Protocol additions, history loading/preparation split, routing updates, and related test adjustments are all in scope per #1317.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/unified-agent-loop-pr1

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@model_gateway/src/routers/openai/responses/route.rs`:
- Around line 133-134: The RequestContext is still built from the original
body.clone(), so downstream tool-loop paths get the unnormalized input; after
calling super::history::prepare_agent_loop_input(&request_body.input) and
assigning request_body.input = prepared.upstream_input, update how the
RequestContext is created: ensure ctx.responses_request() (and the other
occurrence around the similar block at the later lines) uses the
prepared/modified request_body (or the serialized payload derived from it)
instead of the original body.clone(); in short, replace usages of body.clone()
when constructing payload/RequestContext with the prepared request_body so the
propagated payload matches prepare_agent_loop_input's canonicalized
upstream_input.

In `@model_gateway/tests/api/responses_api_test.rs`:
- Around line 647-663: The test currently only checks for absence of
"mcp_list_tools" but still allows a new "mcp_call" plus a final "message" to
pass; update the first assertion that inspects output to assert there are no
items with type "mcp_call" (i.e., change the predicate from "mcp_list_tools" to
"mcp_call" or otherwise assert output.iter().all(|item|
item.get("type").and_then(|v| v.as_str()) != Some("mcp_call"))), so the resume
response cannot contain a new MCP invocation, while keeping the existing
assertion that a final "message" exists.

🪄 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: 52aef8c8-26a6-4edc-bfb1-18be881807bd

📥 Commits

Reviewing files that changed from the base of the PR and between f34fd92 and 3485930.

📒 Files selected for processing (8)

• crates/protocols/src/responses.rs
• crates/protocols/tests/responses.rs
• model_gateway/benches/routing_allocation_bench.rs
• model_gateway/src/routers/grpc/harmony/builder.rs
• model_gateway/src/routers/grpc/regular/responses/conversions.rs
• model_gateway/src/routers/openai/responses/history.rs
• model_gateway/src/routers/openai/responses/route.rs
• model_gateway/tests/api/responses_api_test.rs

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7922bdd448

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: efd6558683

ℹ️ 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".

[coderabbitai]

Actionable comments posted: 2

🤖 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 1800-1848: The test response_input_accepts_mcp_trace_items only
checks serde deserialization but not the protocol validation; after creating the
ResponsesRequest named request, call the validator (request.validate()) and
assert it succeeds (e.g. request.validate().expect("request should validate") or
assert!(request.validate().is_ok())) so missing match arms in the
ResponsesRequest validation can't regress silently; place this assertion
immediately after the deserialization (after the .expect("request should
deserialize")) and before further assertions about ResponseInputOutputItem
variants.

In `@model_gateway/src/routers/openai/responses/history.rs`:
- Around line 384-388: The ResponseInput::Items branch inside
append_current_input is applying normalize_input_item to each incoming item,
which duplicates normalization because prepare_agent_loop_input also normalizes
the stitched transcript; remove the .map(normalize_input_item) call in the
ResponseInput::Items(current_items) handling in append_current_input so it
simply extends items with the incoming items (preserving their original
variants), and let prepare_agent_loop_input retain the single normalization
boundary via normalize_input_item when assembling the final input.

🪄 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: 4f22b052-84da-4814-9207-53fb4ba4cad3

📥 Commits

Reviewing files that changed from the base of the PR and between ecda215 and 4211d52.

📒 Files selected for processing (13)

• crates/protocols/src/responses.rs
• crates/protocols/tests/responses.rs
• model_gateway/benches/routing_allocation_bench.rs
• model_gateway/src/routers/grpc/harmony/builder.rs
• model_gateway/src/routers/grpc/regular/responses/conversions.rs
• model_gateway/src/routers/openai/context.rs
• model_gateway/src/routers/openai/mcp/tool_loop.rs
• model_gateway/src/routers/openai/responses/history.rs
• model_gateway/src/routers/openai/responses/non_streaming.rs
• model_gateway/src/routers/openai/responses/route.rs
• model_gateway/src/routers/openai/responses/streaming.rs
• model_gateway/tests/api/responses_api_test.rs
• model_gateway/tests/routing/test_openai_routing.rs

[coderabbitai]

Actionable comments posted: 3

🤖 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 2038-2110: The test
mcp_call_input_omits_optional_fields_when_unset currently only checks that
optional fields are omitted; extend it to assert the full MCP wire shape for the
McpCall variant by verifying required serialized fields (e.g. "id",
"server_label", "name", "arguments", "output", "status", and that "type" ==
"mcp_call") are present and have the expected values after serializing the
ResponseInputOutputItem::McpCall instance; use the serialized serde_json::Value
(v) to check each key/value so regressions that drop required fields are
detected.

In `@model_gateway/src/routers/grpc/harmony/builder.rs`:
- Around line 1124-1129: The test currently only checks messages.len() == 2 but
should assert that the two surviving messages are the user "hello" and assistant
"done"; update the test after calling
HarmonyBuilder::new().construct_input_messages_with_harmony(&request) to inspect
the returned messages vector and assert each element's role and text (e.g.,
messages[0] is role "user" with content "hello" and messages[1] is role
"assistant" with content "done") using assert_eq! or pattern matches so the test
verifies which specific messages survived rather than only the count.

In `@model_gateway/src/routers/grpc/regular/responses/conversions.rs`:
- Around line 616-618: The test currently only checks chat_req.messages.len();
update it to assert the retained messages' roles and contents to ensure user and
assistant messages survive and MCP trace items are omitted: after creating
chat_req via responses_to_chat(&req), add assertions that chat_req.messages[0]
has role "user" and the expected content string, and that chat_req.messages[1]
has role "assistant" with its expected content (or compare full Message objects
if available), and also assert that none of the messages contains MCP trace
markers; reference responses_to_chat and chat_req.messages to locate where to
add these checks.

🪄 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: 1f7feb3c-3ddb-4d9f-b535-cbc6349d2756

📥 Commits

Reviewing files that changed from the base of the PR and between 4211d52 and 153b3b5.

📒 Files selected for processing (13)

• crates/protocols/src/responses.rs
• crates/protocols/tests/responses.rs
• model_gateway/benches/routing_allocation_bench.rs
• model_gateway/src/routers/grpc/harmony/builder.rs
• model_gateway/src/routers/grpc/regular/responses/conversions.rs
• model_gateway/src/routers/openai/context.rs
• model_gateway/src/routers/openai/mcp/tool_loop.rs
• model_gateway/src/routers/openai/responses/history.rs
• model_gateway/src/routers/openai/responses/non_streaming.rs
• model_gateway/src/routers/openai/responses/route.rs
• model_gateway/src/routers/openai/responses/streaming.rs
• model_gateway/tests/api/responses_api_test.rs
• model_gateway/tests/routing/test_openai_routing.rs

[github-actions]

This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you!

[mergify]

Hi @zhaowenzi, 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

[github-actions]

This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you!

[github-actions]

This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you!