feat(router): add stateful tool bootstrap with integ tests#1401
feat(router): add stateful tool bootstrap with integ tests#1401RohanSogani wants to merge 8 commits into
Conversation
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
RohanSogani
requested review from
CatherineSue,
key4ng and
slin1237
as code owners
github-actions
Bot
added
tests
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a bootstrapping mechanism for stateful tools, such as Code Interpreter and Shell, within the OpenAI Responses API. The changes include a new stateful_tools module, updates to the request context to track bootstrap state, and comprehensive integration tests. The review feedback identifies critical issues where the RequestContext is initialized with the original request body rather than the modified version containing history and memory context. Furthermore, the feedback highlights that the context must be updated after bootstrapping to ensure injected items are preserved across tool call iterations. Finally, it is recommended to use unique UUIDs for injected message identifiers to avoid potential issues with downstream systems.
![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: dd0844cefc
ℹ️ 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".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRouter path for OpenAI Responses now prepares and runs a per-request stateful-tool bootstrap before request transformation, threads bootstrap state through request/context, updates streaming and non-streaming paths to use separate Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router
participant Bootstrapper as StatefulToolBootstrapper
participant Worker as Downstream Worker
participant ToolLoop as MCP ToolLoop
participant Persistence as Persistence
Client->>Router: POST /v1/responses (original request)
Router->>Router: inject history/memory, build RequestContext
Router->>Bootstrapper: ensure_stateful_tool_bootstrap(request, bootstrap_state, context)
Bootstrapper-->>Router: prepared_states + injected_input_items
Router->>Router: prepend injected items into request.input (client_body/request_body)
Router->>Worker: send transformed worker request (uses request_body for MCP, client_body for persistence metadata)
Worker->>ToolLoop: trigger MCP/tool-loop when tools present (ToolLoopState includes bootstrap_state)
ToolLoop->>ToolLoop: execute tool loop using prepared state
Router->>Persistence: persist conversation items using build_persistence_request_body(request_body, client_body)
Note right of Router: bootstrap_state persisted and threaded through tool loop
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/tests/common/mock_worker.rs (1)
1240-1286:⚠️ Potential issue | 🟡 MinorThe recorded-request store isn't isolated across worker lifetimes.
This global store is keyed only by
port, butMockWorkeruses ephemeral ports and never clears the slot onstart()/stop(). If the OS reuses a port across tests,take_recorded_responses_requests_for_port()can pick up stale payloads from an earlier worker and make these new assertions flaky. Clearing the entry on startup/shutdown, or keying by a per-worker UUID instead of the TCP port, would avoid that cross-test leakage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/tests/common/mock_worker.rs` around lines 1240 - 1286, The recorded-request store RESPONSES_REQUEST_STORE is keyed only by port which allows stale data to leak across worker lifetimes; update the design to isolate by a per-worker UUID or clear the stored entry when a MockWorker starts/stops. Concretely, replace or augment RESPONSES_REQUEST_STORE so keys are a worker_id (Uuid) instead of u16 (modify get_responses_request_store, record_responses_request_for_port, take_recorded_responses_requests_for_port to accept and use a Uuid worker_id or provide a clear_recorded_responses_for_port function called from MockWorker::start()/stop), ensure MockWorker generates and exposes a UUID on creation/start and uses that UUID when recording/reading requests, and remove reliance on TCP port as the sole key to prevent cross-test leakage.
🤖 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/non_streaming.rs`:
- Around line 41-45: The destructure of ResponsesPayloadState currently discards
stateful_tool_bootstrap (via stateful_tool_bootstrap: _stateful_tool_bootstrap)
which prevents PreparedToolState produced by route_responses from reaching
execute_tool_loop and later non-streaming tool handling; change the destructure
to preserve the stateful_tool_bootstrap value (e.g., bind it to
stateful_tool_bootstrap) and thread that preserved PreparedToolState through the
non-streaming execution path—ensure
ctx.take_responses_payload().unwrap_or_default() returns the preserved state and
pass stateful_tool_bootstrap into whatever function or context (such as
execute_tool_loop or subsequent non-streaming tool handling code) expects
per-tool state so bootstrap-produced state is available downstream.
In `@model_gateway/src/routers/openai/responses/streaming.rs`:
- Line 686: The prepared bootstrap state (req.stateful_tool_bootstrap) is being
discarded by binding it to _stateful_tool_bootstrap; instead capture it (e.g.,
let stateful_tool_bootstrap = req.stateful_tool_bootstrap) and thread that
PreparedToolState into the streaming execution context used by the spawned tool
loop so the tool loop can access the per-tool prepared state; update the
spawning call and any context/struct (used by route_responses /
ResponsesPayloadState) to accept and propagate stateful_tool_bootstrap rather
than dropping it.
In `@model_gateway/src/routers/openai/stateful_tools.rs`:
- Line 122: The function signature parameters using "bootstrapper: &(dyn
StatefulToolBootstrapper + Send + Sync)" are redundant because
StatefulToolBootstrapper already requires Send + Sync; remove the explicit "+
Send + Sync" and change the parameter to "bootstrapper: &dyn
StatefulToolBootstrapper" in the affected functions (the parameter named
bootstrapper at lines around the definitions that include this type, e.g., where
the function/impl accepts &dyn StatefulToolBootstrapper).
- Around line 62-66: StatefulToolBootstrapResult currently embeds a
StatefulToolBootstrapState whose executed flag is never used because
ensure_stateful_tool_bootstrap always sets bootstrap_state.executed = true and
never reads result.prepared_state.executed; change the result type to return
only the prepared tools (e.g., Vec<ResponseInputOutputItem> or a new
PreparedState struct without the executed bool) instead of
StatefulToolBootstrapState, update ensure_stateful_tool_bootstrap to populate
that narrowed result, and adjust CountingBootstrapper and affected unit-test
fixtures to produce/expect the new simplified result shape.
- Around line 148-158: The Text normalization branch in stateful_tools.rs
currently assigns an empty id (id: String::new()) when converting
ResponseInput::Text into a ResponseInputOutputItem::Message; replace that empty
id with a generated id using generate_id("msg") so the created Message has a
unique non-empty id. Update the match arm in the function handling ResponseInput
(the ResponseInput::Text branch) to set id: generate_id("msg") and ensure the
module imports or can access the generate_id function used elsewhere in the
router so compilation succeeds.
---
Outside diff comments:
In `@model_gateway/tests/common/mock_worker.rs`:
- Around line 1240-1286: The recorded-request store RESPONSES_REQUEST_STORE is
keyed only by port which allows stale data to leak across worker lifetimes;
update the design to isolate by a per-worker UUID or clear the stored entry when
a MockWorker starts/stops. Concretely, replace or augment
RESPONSES_REQUEST_STORE so keys are a worker_id (Uuid) instead of u16 (modify
get_responses_request_store, record_responses_request_for_port,
take_recorded_responses_requests_for_port to accept and use a Uuid worker_id or
provide a clear_recorded_responses_for_port function called from
MockWorker::start()/stop), ensure MockWorker generates and exposes a UUID on
creation/start and uses that UUID when recording/reading requests, and remove
reliance on TCP port as the sole key to prevent cross-test leakage.
🪄 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: 3af1949d-aa09-41a9-840a-3ec8d9562293
📒 Files selected for processing (9)
model_gateway/src/routers/openai/context.rsmodel_gateway/src/routers/openai/mod.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/route.rsmodel_gateway/src/routers/openai/responses/streaming.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/routers/openai/stateful_tools.rsmodel_gateway/tests/api/responses_api_test.rsmodel_gateway/tests/common/mock_worker.rs
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3209bc94f0
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/openai/mcp/tool_loop.rs (1)
812-850:⚠️ Potential issue | 🟠 MajorPrepared bootstrap state does not reach tool execution handlers.
StatefulToolBootstrapStateis stored inToolLoopStateand has aprepared_tool()accessor, but that accessor is never called in production code. The tool execution chain (execute_tool_resolved_result→execute_tool_entry_result) does not receiveStatefulToolBootstrapState, andToolExecutionInput(passed to handlers) contains onlycall_id,tool_name, andarguments—no prepared state. The only production reference is the startup log at line 849.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/mcp/tool_loop.rs` around lines 812 - 850, The prepared StatefulToolBootstrapState is never passed into the tool execution handlers; update the plumbing so the prepared state is propagated from ToolLoopState into the handler input: call ToolLoopState.prepared_tool() where you build per-call execution context in execute_tool_loop and include that prepared state in the ToolExecutionInput (extend ToolExecutionInput to hold an Option<StatefulToolBootstrapState> or reference), then thread that field through execute_tool_resolved_result and execute_tool_entry_result so handlers receive the prepared state; ensure all call sites constructing ToolExecutionInput (and tests) are updated to supply the prepared state from the ToolLoopState.
♻️ Duplicate comments (1)
model_gateway/src/routers/openai/stateful_tools.rs (1)
149-159: 🧹 Nitpick | 🔵 TrivialAvoid the unnecessary
text.clone()in theTextbranch.
*inputis unconditionally replaced two lines below, so the originalStringinsideResponseInput::Text(_)will be dropped. You can move it into the newInputTextpart rather than cloning.♻️ Optional refactor
ResponseInput::Text(text) => { + let text = std::mem::take(text); injected_items.push(ResponseInputOutputItem::Message { id: generate_id("msg"), role: "user".to_string(), - content: vec![ResponseContentPart::InputText { text: text.clone() }], + content: vec![ResponseContentPart::InputText { text }], status: None, phase: None, }); *input = ResponseInput::Items(injected_items); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/stateful_tools.rs` around lines 149 - 159, The Text branch currently clones the string (`text.clone()`); instead take ownership of the String instead of cloning it: extract the owned String from `input` (for example via `let text = match std::mem::take(input) { ResponseInput::Text(t) => t, other => { *input = other; return; } };`) and then push the `ResponseInputOutputItem::Message` using `ResponseContentPart::InputText { text }` (no clone), finally set `*input = ResponseInput::Items(injected_items)` as before; update the `ResponseInput::Text(text)` handling so the original String is moved into `InputText` rather than cloned.
🤖 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/mcp/tool_loop.rs`:
- Around line 62-75: ToolLoopState::new is currently cfg(test) and requires
(original_input, prior_mcp_list_tools_labels), but production code still calls
ToolLoopState::new(original_request.input.clone()); update those production call
sites that pass a single argument to call
ToolLoopState::new_with_bootstrap(original_request.input.clone(),
prior_mcp_list_tools_labels, StatefulToolBootstrapState::default()) (or supply
the correct prior_mcp_list_tools_labels value available in that context), or
alternatively add a public single-argument constructor that forwards to
new_with_bootstrap using StatefulToolBootstrapState::default() to restore a
production-safe path; key symbols: ToolLoopState::new,
ToolLoopState::new_with_bootstrap, StatefulToolBootstrapState::default(), and
the production call sites invoking
ToolLoopState::new(original_request.input.clone()).
In `@model_gateway/src/routers/openai/stateful_tools.rs`:
- Around line 120-139: Add an explicit INVARIANT comment to
ensure_stateful_tool_bootstrap documenting that bootstrap_state.executed is
intentionally only set to true after a successful bootstrap (i.e., after
bootstrapper.bootstrap(...) returns Ok), so a failing bootstrap leaves
executed=false and allows callers to re-run the function; reference
bootstrap_state.executed, bootstrapper.bootstrap, and the fact that injected
items and prepared tools are only applied on success to make the
single-attempt-per-success-per-request contract explicit for future maintainers.
In `@model_gateway/tests/api/responses_api_test.rs`:
- Around line 161-172: The test is brittle because bootstrap_payload_texts
collects every "text" under input[*].content[*] regardless of message role;
update bootstrap_payload_texts to first filter input array items by role (only
include items where item.get("role") is "system" or "user") before extracting
content texts, and update the resume tests that asserted exact equality to
either assert the bootstrap text is present (contains) or assert the first
collected text equals the expected "bootstrap context" so the tests no longer
fail when assistant-function mixing occurs; refer to the bootstrap_payload_texts
function and the resume tests that assert bootstrap payload texts.
---
Outside diff comments:
In `@model_gateway/src/routers/openai/mcp/tool_loop.rs`:
- Around line 812-850: The prepared StatefulToolBootstrapState is never passed
into the tool execution handlers; update the plumbing so the prepared state is
propagated from ToolLoopState into the handler input: call
ToolLoopState.prepared_tool() where you build per-call execution context in
execute_tool_loop and include that prepared state in the ToolExecutionInput
(extend ToolExecutionInput to hold an Option<StatefulToolBootstrapState> or
reference), then thread that field through execute_tool_resolved_result and
execute_tool_entry_result so handlers receive the prepared state; ensure all
call sites constructing ToolExecutionInput (and tests) are updated to supply the
prepared state from the ToolLoopState.
---
Duplicate comments:
In `@model_gateway/src/routers/openai/stateful_tools.rs`:
- Around line 149-159: The Text branch currently clones the string
(`text.clone()`); instead take ownership of the String instead of cloning it:
extract the owned String from `input` (for example via `let text = match
std::mem::take(input) { ResponseInput::Text(t) => t, other => { *input = other;
return; } };`) and then push the `ResponseInputOutputItem::Message` using
`ResponseContentPart::InputText { text }` (no clone), finally set `*input =
ResponseInput::Items(injected_items)` as before; update the
`ResponseInput::Text(text)` handling so the original String is moved into
`InputText` rather than cloned.
🪄 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: 6f1f789a-a30b-4f4a-92f7-af2d3333ad9d
📒 Files selected for processing (7)
model_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/route.rsmodel_gateway/src/routers/openai/responses/streaming.rsmodel_gateway/src/routers/openai/stateful_tools.rsmodel_gateway/tests/api/responses_api_test.rsmodel_gateway/tests/routing/test_openai_routing.rs
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4584e63a8e
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/context.rs`:
- Around line 278-286: The OwnedStreamingContext currently stores two full
ResponsesRequest structs (request_body and client_body) which causes deep clones
for the entire stream; change those fields to Arc<ResponsesRequest> in the
OwnedStreamingContext and update all sites that construct or assign
OwnedStreamingContext (the places around the other occurrences you noted) to
wrap the existing ResponsesRequest in Arc::new(...) and pass Arc clones
(Arc::clone) instead of cloning the whole struct—this preserves the
client/upstream split with cheap reference clones and avoids keeping two
deep-copied payloads for the stream lifetime.
In `@model_gateway/src/routers/openai/responses/history.rs`:
- Around line 364-385: The test
deserialize_items_from_array_drops_reasoning_replay_items currently may pass due
to deserialization failure rather than filtering; update the fixture so the
second "reasoning" item matches the verified summary-based wire shape that
successfully deserializes (so deserialize_items_from_array actually produces a
reasoning variant), then assert that the resulting vector from
deserialize_items_from_array contains only the Message (e.g., parsed.len() == 1
and matches!(parsed[0], ResponseInputOutputItem::Message { .. })), proving the
item was removed by the filter rather than dropped during parse; reference the
test function deserialize_items_from_array_drops_reasoning_replay_items, the
deserializer deserialize_items_from_array, and the
ResponseInputOutputItem::Message variant when making the change.
In `@model_gateway/src/routers/openai/responses/route.rs`:
- Around line 157-165: The early-return when
ctx.components.stateful_tool_bootstrapper() is None in route_responses currently
skips router-error telemetry; before returning from that None branch (where
stateful_tool_bootstrapper is checked), call Metrics::record_router_error(...)
via the router metrics handle (e.g., ctx.metrics() or the existing Metrics
accessor) with an appropriate error identifier (e.g.,
"stateful_tool_bootstrapper_missing") and any relevant tags, then proceed to
return the internal_error as before; update the branch around the
stateful_tool_bootstrapper check to emit this metric prior to the
error::internal_error(...) return.
In `@model_gateway/src/routers/openai/stateful_tools.rs`:
- Around line 234-330: Add a unit test that verifies a failing bootstrap leaves
state and request unchanged: call ensure_stateful_tool_bootstrap with a
bootstrapper that returns an Err, using a non-empty request.tools and a baseline
ResponsesRequest.input (e.g., Text or Items), then assert
bootstrap_state.executed is false and request.input equals the original value;
reference ensure_stateful_tool_bootstrap, StatefulToolBootstrapState,
ResponsesRequest and the bootstrapper type used in other tests (e.g.,
CountingBootstrapper or a new FailingBootstrapper) to locate where to add the
test and how to simulate the error path.
🪄 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: c6ca67d9-9231-4ee6-b1ec-36e8a99b3c9b
📒 Files selected for processing (6)
model_gateway/src/routers/openai/context.rsmodel_gateway/src/routers/openai/responses/history.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/route.rsmodel_gateway/src/routers/openai/responses/streaming.rsmodel_gateway/src/routers/openai/stateful_tools.rs
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78703031ed
ℹ️ 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".
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 `@model_gateway/src/routers/openai/responses/utils.rs`:
- Around line 31-40: The persistence request built in
build_persistence_request_body currently clones request_body then copies
conversation, store, and user from client_body, but it omits copying
client_body.previous_response_id so the persisted record can lose its resume
link; update build_persistence_request_body to also copy
persistence_body.previous_response_id = client_body.previous_response_id (or use
clone_from if it's an Option/String type) immediately after copying
conversation/store/user so the persisted ResponsesRequest preserves the caller's
previous_response_id.
🪄 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: c6457b46-b9da-437b-9192-46a4c58e3b00
📒 Files selected for processing (3)
model_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/streaming.rsmodel_gateway/src/routers/openai/responses/utils.rs
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38bfae040c
ℹ️ 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".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
model_gateway/src/routers/openai/responses/utils.rs (1)
31-49:⚠️ Potential issue | 🟠 MajorRestore
previous_response_idhere as well.This helper reattaches caller-owned persistence metadata, but it still drops
previous_response_id. That breaks the resume link for persisted requests in both streaming and non-streaming paths.🩹 Proposed fix
persistence_body .conversation .clone_from(&client_body.conversation); persistence_body.store = client_body.store; + persistence_body + .previous_response_id + .clone_from(&client_body.previous_response_id); persistence_body.user.clone_from(&client_body.user);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/responses/utils.rs` around lines 31 - 49, build_persistence_request_body currently copies conversation, store, user, and input from client_body but omits restoring previous_response_id, breaking resume linking; modify build_persistence_request_body to also copy previous_response_id from client_body into persistence_body (i.e., set persistence_body.previous_response_id = client_body.previous_response_id or use clone_from as with user/conversation) so persisted requests retain the caller-owned resume link.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@model_gateway/src/routers/openai/responses/utils.rs`:
- Around line 31-49: build_persistence_request_body currently copies
conversation, store, user, and input from client_body but omits restoring
previous_response_id, breaking resume linking; modify
build_persistence_request_body to also copy previous_response_id from
client_body into persistence_body (i.e., set
persistence_body.previous_response_id = client_body.previous_response_id or use
clone_from as with user/conversation) so persisted requests retain the
caller-owned resume link.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e65997c-ea8c-405b-83da-16b504913048
📒 Files selected for processing (1)
model_gateway/src/routers/openai/responses/utils.rs
RohanSogani
changed the title
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4413a2b735
ℹ️ 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".
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 769a2e7014
ℹ️ 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".
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
1 participant
Description
Resolves #1297
Problem
SMG’s OpenAI-compatible Responses path did not have a generic way to prepare request-scoped state for stateful built-in tools before the first model call.
That left a gap for tools like:
code_interpretershellBefore this change, the router could not cleanly do all of the following in one place:
Without this layer, the next provider-specific/runtime integration would either have to:
That would make the upstream design brittle and tightly coupled to one provider/runtime implementation.
Solution
Add a generic request-scoped stateful tool bootstrap layer to the OpenAI Responses router.
This change introduces:
StatefulToolBootstrapperinterface.StatefulToolBootstrapState.client_body,request_body, andpersistence_body.Request-body roles:
client_body: the original caller request and metadata.request_body: the normalized execution request sent upstream.persistence_body: the storage request that keeps caller input/metadata without replay-expanded or bootstrap-expanded context.The important architectural boundary is preserved:
Changes
model_gateway/src/routers/openai/stateful_tools.rs.code_interpreterandshell.PreparedToolStatewith opaque JSON payload storage.StatefulToolBootstrapStateto track whether bootstrap has executed and prepared tool state entries for the request.StatefulToolBootstrapContextso bootstrap implementations receive request headers, storage request context, memory execution context, and tenant request metadata.StatefulToolBootstrapperasync trait.NoOpStatefulToolBootstrapperas the default safe upstream implementation.ResponsesRequest.tools.model_gateway/src/routers/openai/responses/route.rs.ResponsesComponentsto carry the bootstrapper.ResponsesPayloadStateto carry bootstrap state.RequestContextandOwnedStreamingContext.RequestContextfrom the original client request so caller metadata is preserved for response patching and persistence.conversation,previous_response_id,store, anduserfrom the caller request.downstream worker call, provider validation before bootstrap side effects, and persistence behavior for
previous_response_idand root requests.Test Plan
Repro from repo root: