feat(protocols): implement P2 top-level ResponsesRequest fields - #1278
Merged
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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:
📝 WalkthroughWalkthroughAdded optional Responses request fields (prompt, prompt_cache_key, prompt_cache_retention, safety_identifier, stream_options, context_management), new protocol types and StreamOptions defaulting, propagated these fields through request-building and Responses→Chat conversion logic, and updated tests for serialization and streaming behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as ModelGateway Router
participant Protocols as Protocol Types
participant Upstream as Responses API
rect rgba(200,230,255,0.5)
Client->>Router: send ResponsesRequest (may include prompt, prompt_cache_key, prompt_cache_retention, safety_identifier, stream_options, context_management)
Router->>Protocols: validate/serialize request fields
Protocols-->>Router: serialized ResponsesRequest
Router->>Upstream: forward ResponsesRequest
Upstream-->>Router: stream/response events
Router->>Client: relay responses (including streaming)
Router->>Router: build_next_request(...) preserves prompt/cache/stream/context for multi-turn
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
grpc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/tests/api/responses_api_test.rs`:
- Around line 107-112: The tests repeatedly construct ResponsesRequest with many
optional fields set to None; create a shared helper like fn
base_responses_request() -> ResponsesRequest that returns a ResponsesRequest
with common defaults (prompt: None, prompt_cache_key: None,
prompt_cache_retention: None, safety_identifier: None, stream_options: None,
context_management: None, etc.), then update only the fields each test needs
(using struct update syntax or a small builder) in the tests that currently
construct ResponsesRequest (references: ResponsesRequest occurrences in these
test sections). Replace the repeated None-initializations in each test with
calls to base_responses_request() and per-test overrides to reduce boilerplate
and future churn.
🪄 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: 39c58428-96fc-49b8-8d41-1797548134ab
📒 Files selected for processing (6)
crates/protocols/src/common.rscrates/protocols/src/responses.rsmodel_gateway/src/routers/grpc/regular/responses/common.rsmodel_gateway/src/routers/grpc/regular/responses/conversions.rsmodel_gateway/tests/api/responses_api_test.rsmodel_gateway/tests/spec/chat_completion.rs
Comment on lines
+107
to
+112
| prompt: None, | ||
| prompt_cache_key: None, | ||
| prompt_cache_retention: None, | ||
| safety_identifier: None, | ||
| stream_options: None, | ||
| context_management: None, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a shared test request builder to remove repeated None boilerplate.
The new optional fields are correctly set, but this repetition will make future request-shape updates noisy. A small helper that returns a base ResponsesRequest and per-test overrides would reduce churn.
Also applies to: 305-310, 435-440, 566-571, 621-626, 743-748, 884-889, 933-938, 1080-1085, 1194-1199, 1353-1358, 1539-1544, 1823-1828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/tests/api/responses_api_test.rs` around lines 107 - 112, The
tests repeatedly construct ResponsesRequest with many optional fields set to
None; create a shared helper like fn base_responses_request() ->
ResponsesRequest that returns a ResponsesRequest with common defaults (prompt:
None, prompt_cache_key: None, prompt_cache_retention: None, safety_identifier:
None, stream_options: None, context_management: None, etc.), then update only
the fields each test needs (using struct update syntax or a small builder) in
the tests that currently construct ResponsesRequest (references:
ResponsesRequest occurrences in these test sections). Replace the repeated
None-initializations in each test with calls to base_responses_request() and
per-test overrides to reduce boilerplate and future churn.
Member
Author
There was a problem hiding this comment.
Thanks. Agreed this is repetition, but a shared test-fixture builder is genuinely out of P2's scope — P2's charter is protocol schema additions; test-infrastructure refactoring across 13 callsites would balloon the blast radius. Tracking as a follow-up for a dedicated refactor(tests) PR. Leaving as-is.
There was a problem hiding this comment.
@slin1237 Understood — keeping P2 focused on protocol schema additions is a sound call. A dedicated refactor(tests) PR targeting the shared fixture builder across those 13 callsites makes more sense than expanding the blast radius here. Feel free to track it as a follow-up when the time is right.
🧠 Learnings used
Learnt from: key4ng
Repo: lightseekorg/smg PR: 1106
File: e2e_test/responses/test_sampling_params.py:20-47
Timestamp: 2026-04-15T04:44:18.006Z
Learning: In repo lightseekorg/smg, `ResponsesResponse` proto only echoes back `temperature`, `top_p`, and `max_output_tokens`. Extension sampling fields (`top_k`, `min_p`, `frequency_penalty`, `presence_penalty`, `repetition_penalty`) are **not** part of the response schema and are therefore not present in `resp.*` in E2E tests (e.g., `e2e_test/responses/test_sampling_params.py`). Do not flag the absence of assertions for these fields in E2E response-sampling tests; builder pass-through for these fields is validated by unit tests in `crates/grpc_client/src/` instead.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 807
File: model_gateway/src/routers/openai/responses/streaming.rs:821-855
Timestamp: 2026-03-18T21:57:03.433Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/openai/responses/streaming.rs: The early `return` statements inside `handle_streaming_with_tool_interception` (on `tx` send failures in `forward_streaming_event`, `send_mcp_list_tools_events`, and the `is_in_progress`/`mcp_list_tools_sent` branches) are pre-existing behavior that predates PR `#807`. They cause the persistence phase (final response / conversation-backed storage writes at the end of the tool loop) to be skipped when the client disconnects mid-stream with `store=true` or a conversation-backed request. This is a known pre-existing gap in the MCP streaming path, not a regression introduced by the storage context header changes in PR `#807`.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 1163
File: e2e_test/responses/test_tools_call.py:1126-1133
Timestamp: 2026-04-16T17:20:26.745Z
Learning: In repo lightseekorg/smg, `TestToolChoiceLocal.test_previous_response_id_mcp_binding_behavior_streaming` in `e2e_test/responses/test_tools_call.py` unconditionally calls `pytest.skip(...)` because regular gRPC MCP streaming responses are not persisted for `previous_response_id` resume (the response ID is emitted but the final response is not stored). This skip is intentional and kept broad for consistency; the persistence gap will be addressed in a dedicated follow-up PR. Do not flag this skip as overly broad or suggest narrowing it to seed from non-streaming responses.
Learnt from: vschandramourya
Repo: lightseekorg/smg PR: 978
File: model_gateway/src/routers/grpc/regular/streaming.rs:2362-2363
Timestamp: 2026-03-31T02:28:31.317Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/grpc/regular/streaming.rs: When a stop sequence is detected mid-stream in `process_completion_streaming_chunks` (and equivalently in `process_streaming_chunks` for chat), the synthetic final-chunk send after a local stop match intentionally uses `let _ = tx.send(...)` (best-effort). The loop must fall through to the `ProtoResponseVariant::Complete` arm to record streaming metrics and call `mark_completed()`. Propagating a channel-closed error at this point would skip metrics recording and leave the request in a dangling state. The backend stream is already locally exhausted at the stop-match site (no wasted capacity). Do not flag this as a silent error or suggest converting it to a propagated `?` error.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 1244
File: crates/protocols/src/builders/responses/response.rs:126-143
Timestamp: 2026-04-20T16:55:01.974Z
Learning: In repo lightseekorg/smg, `completed_at: Option<i64>` on `ResponsesResponse` (added in PR `#1244`, `crates/protocols/src/builders/responses/response.rs`) is intentionally NOT populated by any terminal-response builder in PR `#1244`. Wiring the gRPC regular (`conversions.rs`, `streaming.rs`) and Harmony (`processor.rs`, `non_streaming.rs`) terminal builders to call `.completed_at(...)` before `.build()` is explicitly deferred to BGM-PR-07 (`Worker Execution Core + Retry + Cancellation`) per `2026-04-17-background-mode-task-breakdown.md:170-193`. Do not re-flag `completed_at: None` in terminal responses as a missing population until BGM-PR-07 lands.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs:206-211
Timestamp: 2026-02-21T02:31:17.841Z
Learning: Repo: lightseekorg/smg
File: model_gateway/src/routers/grpc/client.rs
Context: GrpcClient::embed match arm
Learning: The catch-all panic in GrpcClient::embed for mismatched client/request types or unsupported embedding backends is intentional to catch invariant violations and unsupported configurations at development time. Converting this path to a returned error is out of scope for PR `#489`.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs:189-194
Timestamp: 2026-02-21T02:32:13.396Z
Learning: Repo: lightseekorg/smg PR: 489
File: model_gateway/src/routers/grpc/client.rs
Context: GrpcClient::generate fallback match arm
Learning: The catch-all panic for mismatched client/request types in GrpcClient::generate is intentional to enforce pipeline invariants; do not convert this to a returned error in PR `#489` or similar lint-only changes.
Learnt from: vschandramourya
Repo: lightseekorg/smg PR: 953
File: model_gateway/src/routers/grpc/regular/processor.rs:765-780
Timestamp: 2026-03-27T23:46:40.172Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/grpc/regular/processor.rs: The finish_reason parsing in `process_non_streaming_completion_response` is intentionally NOT extracted to `utils::parse_finish_reason`. The completions endpoint uses OpenAI-style `Option<String>` from `openai_protocol::completion::CompletionChoice`, while `utils::parse_finish_reason` maps to the typed generate-endpoint format. These are different API contracts; do not flag the inline finish_reason parsing block in the completions processor as duplicate code or request extraction into the shared utility.
Learnt from: vschandramourya
Repo: lightseekorg/smg PR: 978
File: model_gateway/src/routers/grpc/regular/streaming.rs:2275-2276
Timestamp: 2026-03-31T03:43:30.981Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/grpc/regular/streaming.rs: `logprobs: None` in `CompletionStreamChoice` within `process_completion_streaming_chunks` is intentional. Per-token logprob streaming is deferred across all streaming endpoints (completions, generate) because it requires backend-side plumbing not yet available. The same `// TODO: wire per-token logprob streaming` deferral exists in `process_generate_streaming` (around line 702). Non-streaming completions has the same TODO. Do not flag `logprobs: None` or the TODO comment in the completions or generate streaming paths as a missing feature or silent data loss until a dedicated follow-up PR wires backend per-token logprob support.
Learnt from: pallasathena92
Repo: lightseekorg/smg PR: 406
File: model_gateway/src/routers/openai/realtime/rest.rs:173-189
Timestamp: 2026-03-04T02:13:55.479Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/openai/realtime/rest.rs: The proxy_response function intentionally forwards only the Content-Type header from upstream responses. For the realtime REST token-generation endpoints (client_secrets, sessions, transcription_sessions), the response is a simple JSON body (ephemeral token), so Content-Type is sufficient. Upstream rate-limit headers (x-ratelimit-*, retry-after) are not forwarded because the gateway has its own rate limiting. x-request-id forwarding is a deferred nice-to-have, not a correctness requirement.
Learnt from: vschandramourya
Repo: lightseekorg/smg PR: 915
File: model_gateway/src/routers/grpc/client.rs:387-423
Timestamp: 2026-03-26T17:06:14.307Z
Learning: In repo lightseekorg/smg, in `model_gateway/src/routers/grpc/client.rs` and the corresponding backend builders (`crates/grpc_client/src/sglang_scheduler.rs`, `vllm_engine.rs`, `trtllm_service.rs`): The per-backend divergence in handling `CompletionRequest.max_tokens == None` is intentional. SGLang and vLLM pass `None` through to their proto builders, while TRT-LLM falls back to `16`. This matches the pre-existing per-backend pattern used in the chat/messages request builders. Do not flag this divergence as a bug or request normalization at the `build_completion_request` dispatcher layer in `client.rs`.
Learnt from: XinyueZhang369
Repo: lightseekorg/smg PR: 723
File: model_gateway/tests/api/interactions_api_test.rs:316-346
Timestamp: 2026-03-11T05:32:45.536Z
Learning: In repo lightseekorg/smg, `test_interactions_multiple_workers` in `model_gateway/tests/api/interactions_api_test.rs` intentionally only verifies that requests succeed with multiple Gemini backends (connectivity), not load distribution. Load distribution across workers is covered separately in `tests/routing/load_balancing_test.rs`. Do not flag the absence of per-worker request count assertions in this test as a gap — distribution verification is deferred to a follow-up PR once the Gemini router implementation matures.
Learnt from: vschandramourya
Repo: lightseekorg/smg PR: 964
File: model_gateway/src/routers/grpc/pd_router.rs:233-240
Timestamp: 2026-03-28T04:56:07.632Z
Learning: In repo lightseekorg/smg, `GrpcPDRouter` (model_gateway/src/routers/grpc/pd_router.rs) intentionally passes `&self.retry_config` (the router-level default) to `RetryExecutor::execute_response_with_retry` for ALL endpoints (generate, chat, messages, completion). Per-model retry overrides via `WorkerRegistry::get_retry_config` are NOT applied in the PD router — that pattern is used only in `GrpcRouter` (regular mode). Adding per-model retry support to individual PD endpoints in isolation would be inconsistent; any such change must cover all PD endpoints in a dedicated follow-up PR. Do not flag the missing per-model retry lookup in any single PD router endpoint as a bug.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 1163
File: model_gateway/src/routers/common/mcp_utils.rs:56-67
Timestamp: 2026-04-17T18:06:31.006Z
Learning: In repo lightseekorg/smg, `inject_mcp_output_items` in `model_gateway/src/routers/common/mcp_utils.rs` (used by gRPC regular and Harmony response paths) does NOT need to filter `existing` output items (from `std::mem::take(output)`) with `is_client_visible_output_item` before appending them back. Unlike the OpenAI path (`inject_client_visible_mcp_output_items` in `crates/mcp/src/core/session.rs`), the gRPC response paths do not pre-populate `response.output` with internal `FunctionToolCall` entries before this helper is called, so there is no internal-item leakage risk. The e2e assertions in `e2e_test/responses/test_tools_call.py` confirm this. Do not apply the OpenAI-path filtering requirement to `inject_mcp_output_items` in the gRPC paths.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: multimodal/tests/vision_golden_tests.rs:402-404
Timestamp: 2026-02-21T02:39:23.481Z
Learning: Repo lightseekorg/smg — For clippy-only/lint-enforcement PRs (e.g., PR `#489`), do not replace unwrap() with expect() across tests/benches when a crate-level `#![expect(clippy::unwrap_used)]` is present. Such per-call swaps are treated as out-of-scope stylistic changes. Example: multimodal/tests/vision_golden_tests.rs.
Learnt from: TingtingZhou7
Repo: lightseekorg/smg PR: 1057
File: model_gateway/src/routers/openai/mcp/tool_loop.rs:856-885
Timestamp: 2026-04-08T00:08:05.944Z
Learning: In repo lightseekorg/smg, `sanitize_builtin_tool_arguments` in `model_gateway/src/routers/openai/mcp/tool_loop.rs` intentionally drops image-generation options (size, quality, background, output_format, compression) when handling `ResponseFormat::ImageGenerationCall`, keeping only `model` (hardcoded `IMAGE_MODEL`) and `revised_prompt`. This is a deliberate scoped decision for the initial image-generation tool integration; per-option overrides/defaults are planned for a follow-up PR. Do not flag the truncation as a bug or request preservation of extra fields.
Learnt from: CatherineSue
Repo: lightseekorg/smg PR: 936
File: model_gateway/src/routers/gemini/router.rs:82-90
Timestamp: 2026-03-30T18:52:19.689Z
Learning: In repo lightseekorg/smg, the Gemini router's (`model_gateway/src/routers/gemini/router.rs`) use of `model_id` for per-model retry config lookup in `route_interactions` has a pre-existing limitation: when the request carries only an agent identifier, `get_retry_config(agent_id)` finds no match because agent→model resolution happens later inside `driver::execute` (in `worker_selection.rs`). This same limitation affects policy lookup and worker selection in the Gemini router. It is NOT introduced by PR `#936` and is a known accepted gap. The Gemini router is not actively used currently. Do not flag this as a bug introduced by per-model retry config changes to the Gemini router.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 1250
File: model_gateway/src/routers/openai/responses/history.rs:343-398
Timestamp: 2026-04-20T09:00:17.756Z
Learning: In repo lightseekorg/smg, `mcp_call_output_to_upstream_items` in `model_gateway/src/routers/openai/responses/history.rs` intentionally uses a redundant `item.get("output")` double-lookup and does not preserve the `error` field from stored `mcp_call` items during upstream replay (producing `"null"` instead). This is a deliberate scoping decision in PR `#1250`: the exercised paths always carry an `output` value, and widening replay behavior to handle missing-output/error cases is deferred to a follow-up PR. Do not re-flag the redundant lookup or the silent error discard as blocking issues until that follow-up work is done.
Learnt from: zhoug9127
Repo: lightseekorg/smg PR: 1061
File: model_gateway/src/routers/openai/mcp/tool_loop.rs:846-850
Timestamp: 2026-04-08T16:26:08.331Z
Learning: In repo lightseekorg/smg, `build_transformed_mcp_call_item` in `model_gateway/src/routers/openai/mcp/tool_loop.rs` intentionally omits the `server_label` field from transformed output items when the `ResponseFormat` is a builtin type (`WebSearchCall`, `CodeInterpreterCall`, `FileSearchCall`). This invariant is enforced by the test `build_transformed_mcp_call_item_does_not_add_server_label_for_builtin_formats`. As a result, the label-based filter `is_internal_mcp_response_item` (which checks `server_label`) will never accidentally hide builtin-routed call items — it is safe to rely on the absence of `server_label` for builtin items without adding an explicit type-gate. Do not flag `is_internal_mcp_response_item` as potentially hiding builtin output.
Learnt from: XinyueZhang369
Repo: lightseekorg/smg PR: 723
File: model_gateway/tests/api/interactions_api_test.rs:251-282
Timestamp: 2026-03-11T23:26:31.100Z
Learning: In repo lightseekorg/smg, the Gemini-specific header path (apply_provider_headers emitting x-goog-api-key for googleapis.com URLs, and extract_gemini_auth_header) is intentionally not covered by the mock-server integration tests in model_gateway/tests/api/interactions_api_test.rs (PR `#723`). Coverage against a real Gemini backend URL will be added in a follow-up e2e test PR. Do not flag the absence of this test coverage as a gap in PR `#723` or related PRs.
Learnt from: pallasathena92
Repo: lightseekorg/smg PR: 687
File: model_gateway/src/routers/openai/realtime/webrtc.rs:238-289
Timestamp: 2026-03-11T01:29:56.655Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/openai/realtime/webrtc.rs: `Metrics::record_router_request` is already emitted in router.rs (around line 1152) before `handle_realtime_webrtc` is called, and `Metrics::record_router_error` is emitted inside `handle_realtime_webrtc` for the no-workers case. The missing instrumentation is success/duration recording after `setup_and_spawn_bridge` returns — this is a metrics improvement deferred to a follow-up PR, not a correctness gap. Do not flag missing success/duration metrics as a blocking issue for PR `#687`.
Learnt from: CatherineSue
Repo: lightseekorg/smg PR: 1031
File: model_gateway/src/routers/grpc/regular/streaming.rs:1579-1600
Timestamp: 2026-04-03T05:31:33.707Z
Learning: In repo lightseekorg/smg, `model_gateway/src/routers/grpc/regular/streaming.rs` (`process_messages_streaming_chunks`): The reasoning parser is gated on explicit user opt-in (`ThinkingConfig::Enabled`) via `separate_reasoning = matches!(&original_request.thinking, Some(ThinkingConfig::Enabled { .. }))`, NOT on `thinking_override` (which only controls whether `mark_reasoning_started()` is called on the parser, not whether the parser runs at all). Chat API uses the `separate_reasoning` field on `ChatCompletionRequest` instead. Do not suggest removing or replacing the `ThinkingConfig::Enabled` gate in the Messages path.
Learnt from: vschandramourya
Repo: lightseekorg/smg PR: 840
File: model_gateway/src/routers/grpc/pipeline.rs:645-675
Timestamp: 2026-03-24T00:18:24.771Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/grpc/pipeline.rs: The `FinalResponse::Completion` match arm in `execute_completion` is intentionally unreachable in PR `#840`. `PreparationStage` and `ResponseProcessingStage` in the regular pipeline currently reject `RequestType::Completion` with "wrong_pipeline" errors, so no stage sets `FinalResponse::Completion` yet. This is deliberate scaffolding — completion-specific pipeline stages (preparation + response processing) will be added in follow-up PRs to make the match arm reachable. The `#[expect(dead_code, reason = "Completion pipeline entrypoint is introduced before later stacked PRs wire the router to call it")]` attribute on `execute_completion` documents this intent. Do not flag the match arm as unreachable dead code in PR `#840` or similar incremental completions PRs.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 938
File: model_gateway/src/routers/responses/handlers.rs:36-37
Timestamp: 2026-03-27T00:45:42.263Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/responses/handlers.rs: The `delete_response` handler intentionally does a get_response existence check followed by delete_response (two-step, TOCTOU gap). Making delete_response idempotent (i.e., returning success when the record is already absent) requires a storage interface change and is deferred to a follow-up PR. Do not flag this as a blocking issue.
Learnt from: CatherineSue
Repo: lightseekorg/smg PR: 791
File: model_gateway/src/routers/grpc/harmony/stages/preparation.rs:118-124
Timestamp: 2026-03-17T20:14:15.295Z
Learning: In repo lightseekorg/smg, file model_gateway/src/routers/grpc/harmony/stages/preparation.rs (prepare_chat): The condition `if constraint.is_some() && body_ref.response_format.is_some()` that clears `response_format` only fires when `response_format` was the source of the structural-tag constraint. Protocol-level validation and the previous pipeline stage ensure that a request carrying both a tool constraint and a `response_format` is rejected before reaching this stage. Therefore the comment "If response_format was converted to a structural tag, clear it..." accurately describes the runtime behavior.
Learnt from: XinyueZhang369
Repo: lightseekorg/smg PR: 399
File: protocols/src/interactions.rs:505-509
Timestamp: 2026-02-19T03:08:50.192Z
Learning: In code reviews for Rust projects using the validator crate (v0.20.0), ensure that custom validation functions for numeric primitive types (e.g., f32, i32, u32, i16, etc.) accept the value by value, not by reference. Example: fn validate(value: f32) { ... }. The validator derive macro has a hardcoded list of numeric types that are passed by value, while all other types are passed by reference. Apply this guideline whenever validating numeric fields to align with the derive macro behavior.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: model_gateway/src/core/token_bucket.rs:58-63
Timestamp: 2026-02-21T02:30:51.443Z
Learning: For lint-only/Clippy enforcement PRs in this repository, avoid introducing behavioral changes (e.g., new input validation or logic changes). Treat such PRs as non-functional changes and plan a separate follow-up issue/PR for hardening or behavior changes. This applies broadly to Rust files across the repo; during review, focus on lint/style corrections and clearly note any intentional exceptions.
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: protocols/src/responses.rs:928-931
Timestamp: 2026-02-21T02:36:00.882Z
Learning: In Rust code across the repository, use the marker INVARIANT: to document assumptions in safe code. Reserve SAFETY: for explaining why unsafe blocks are sound. This improves clarity of invariants and safety reasoning. Example reference: protocols/src/responses.rs near validate_tool_choice_with_tools().
Learnt from: slin1237
Repo: lightseekorg/smg PR: 489
File: mesh/src/sync.rs:83-83
Timestamp: 2026-02-21T02:37:01.416Z
Learning: General Rust formatting rule: format! with implicit captures only supports simple identifiers, not full expressions like {state.model_id}. For cases where you want to interpolate a field or expression, bind the value first and interpolate the binding, e.g., let model_id = &state.model_id; and then use format!("policy:{}", model_id). In the specific file mesh/src/sync.rs, prefer format!("policy:{}", state.model_id) or bind to a local variable if you need named interpolation, to keep clarity and avoid unintended captures.
Learnt from: zhaowenzi
Repo: lightseekorg/smg PR: 807
File: model_gateway/src/middleware.rs:61-81
Timestamp: 2026-03-18T21:32:00.041Z
Learning: In Rust code using the http crate, HeaderMap::get() is effectively case-insensitive because HeaderName normalizes keys to lowercase on insertion and lookup. Do not require or perform explicit .to_lowercase() before HeaderMap::get() calls. Mark as not a concern for case-sensitivity in lookups; only consider normalization when inserting or comparing via HeaderName, not in lookups.
Learnt from: key4ng
Repo: lightseekorg/smg PR: 867
File: tui/src/app.rs:798-813
Timestamp: 2026-03-22T20:13:55.778Z
Learning: In this repo (lightseekorg/smg), treat the workspace `Cargo.toml`’s `package.rust-version` (MSRV) as the source of truth (e.g., `rust-version = "1.85"`). When reviewing Rust changes, do not flag usage of Rust language/library features that were stabilized on or before the MSRV (e.g., `Option::is_none_or`, stabilized in 1.82, is compatible with an MSRV of 1.85). Always verify the MSRV from the workspace `Cargo.toml` rather than relying on issue templates.
![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: 1730e5c62d
ℹ️ 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".
Comment on lines
+188
to
+190
| Some(StreamOptions { | ||
| include_usage: Some(true), | ||
| ..StreamOptions::default() |
There was a problem hiding this comment.
In responses_to_chat, the new ResponsesRequest.stream_options is overwritten with a fresh StreamOptions literal, so caller-provided values are discarded. A streaming request that sets "include_obfuscation": false is converted with include_obfuscation: None, meaning the new field added in this commit has no effect on the regular gRPC Responses path. Please merge req.stream_options into the constructed value (while still forcing include_usage if needed) instead of resetting from Default.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Fixed in 6cdf09a. responses_to_chat now clones the caller's stream_options forward and only backfills include_usage=Some(true) when unset, so include_obfuscation=false (and any future StreamOptions fields) round-trip faithfully. Added three regression tests covering the obfuscation case, caller-set include_usage, and the non-streaming drop-through.
Comment on lines
+813
to
+816
| /// Streaming-only options. Spec: body param `stream_options`. | ||
| /// On the Responses API the only documented field is `include_obfuscation`. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub stream_options: Option<StreamOptions>, |
There was a problem hiding this comment.
The new stream_options field is documented as streaming-only, but no cross-parameter validation was added for Responses requests. As a result, requests with stream: false and stream_options set are accepted and then silently dropped during chat conversion, which differs from the explicit validation behavior already enforced in Chat/Completions. Adding a stream_options-requires-stream check in validate_responses_cross_parameters would prevent this silent no-op.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Thanks for catching. This is legitimate validation hygiene, but cross-parameter validation is deliberately out of P2's schema-only scope — the P-group tasks close wire-format gaps, while semantic validation (including cross-param rules like "stream_options requires stream=true") belongs to a separate validation pass that hasn't been carved as an audit task yet. Filing as a follow-up rather than expanding P2 mid-flight.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — clean additive change with solid test coverage.
Summary: 0 🔴 Important · 0 🟡 Nit · 1 🟣 Pre-existing (not in diff)
The six new ResponsesRequest fields (prompt, prompt_cache_key, prompt_cache_retention, safety_identifier, stream_options, context_management) are correctly modeled, serialized, defaulted, and propagated through the gRPC tool-loop path. The new types (PromptCacheRetention, ContextManagementEntry, ContextManagementType) are straightforward serde data types with appropriate fail-fast behavior for unknown variants. StreamOptions extension with include_obfuscation + Default derive is handled cleanly in both the conversion and test paths.
🟣 Pre-existing note (outside this diff): persistence_utils.rs:145 and utils.rs:90-98 map original_body.user → safety_identifier in stored responses / response patching. Now that ResponsesRequest has an explicit safety_identifier field (documented as "replaces user"), a client that sends safety_identifier without user will have it silently dropped. Worth a follow-up to prefer safety_identifier.or(user).
slin1237
force-pushed
the
feat/audit-p2-top-level
branch
from
April 21, 2026 05:26
1730e5c to
faa6d24
Compare
There was a problem hiding this comment.
Review Summary (incremental — force-push fallback to full diff)
Reviewed all 6 changed files (+312 / −3). Clean, well-structured PR that adds the 6 missing Responses API top-level fields with proper serde attributes, supporting types, round-trip tests, and multi-turn propagation.
Existing comments — my assessment
| Bot | Location | Severity | My take |
|---|---|---|---|
| chatgpt-codex-connector | conversions.rs:190 |
P1 | Agree partially. stream_options caller values are indeed discarded during Responses→Chat conversion, but include_obfuscation is a Responses-only concept with no Chat equivalent. The field IS correctly propagated through build_next_request for multi-turn continuations. PR explicitly scopes out runtime wiring. More accurately a P2 (known gap, not a regression). |
| chatgpt-codex-connector | responses.rs:816 |
P2 | Agree. Missing stream_options-requires-stream cross-parameter validation creates a parity gap with Chat (chat.rs:367) and Completions (completion.rs:168). Worth adding in a follow-up. |
| coderabbitai | responses_api_test.rs:112 |
Nit | Reasonable suggestion for a shared test builder, but low priority — the struct-literal pattern is compile-forced and catches missing fields. |
What I checked and found clean
- Type definitions:
PromptCacheRetentionserde renames (in-memory,24h) match spec.ContextManagementTypeclosed enum (no#[serde(other)]) aligns with P5 fail-fast direction. StreamOptionsextension:Defaultderive +include_obfuscation: Option<bool>withskip_serializing_if— all 3 construction sites updated with..StreamOptions::default().build_next_request(common.rs): 6 new fields moved (not cloned), consistent with existing pattern. Multi-turn continuations correctly preserve prompt template, cache key, safety ID, streaming options, and context management.- Tests: Round-trip (full payload + both enum variants), absent-field omission (
skip_serializing_ifverification), and 13 compile-forced test fixtures. Good coverage. - No new security concerns: All fields are
Option<_>pass-through withskip_serializing_if. No injection surface.
Severity count (new issues this review)
🔴 Important: 0 · 🟡 Nit: 0 · 🟣 Pre-existing: 0
No new issues found beyond what's already flagged.
slin1237
added a commit
that referenced
this pull request
Apr 21, 2026
… bot feedback)
What
----
- model_gateway/src/routers/grpc/regular/responses/conversions.rs:
- `responses_to_chat` no longer overwrites `ResponsesRequest.stream_options`
with a fresh `StreamOptions { include_usage: Some(true), ..default() }`
literal. The caller's `stream_options` is now cloned forward and only
`include_usage` is defaulted to `Some(true)` when the caller did not set
it. Non-streaming requests still intentionally drop `stream_options`.
- Drop the now test-only `StreamOptions` import from the module-level use
block and re-import it inside `mod tests` so the non-test build does not
emit `unused_import`.
- Add three regression tests:
* `test_stream_options_include_obfuscation_roundtrip` — proves
`include_obfuscation: Some(false)` survives the conversion and
`include_usage` still defaults to `Some(true)` when unset.
* `test_stream_options_caller_include_usage_preserved` — proves the
caller's `include_usage: Some(false)` is not clobbered.
* `test_stream_options_non_streaming_dropped` — confirms `stream=false`
yields `stream_options: None` even when the caller set fields.
Why
---
Codex bot (P1 Major) on PR #1278 flagged that a streaming request with
`stream_options.include_obfuscation=false` was being converted to a
ChatCompletionRequest whose `include_obfuscation` silently became `None`,
discarding caller intent. This is a wire-semantics bug introduced by the P2
schema additions: adding `stream_options` to `ResponsesRequest` without
threading it through the gRPC conversion path left the field accepted but
ignored.
How
---
Switch the literal construction to
`let mut opts = req.stream_options.clone().unwrap_or_default();` followed by
an `if opts.include_usage.is_none()` backfill. This preserves every caller-
supplied field (including future `StreamOptions` additions via `Default`)
while keeping the existing `include_usage=true` behaviour internal pipeline
consumers rely on for end-of-stream usage emission.
Scope
-----
Bug-fix only; no protocol schema changes, no new CLI flags, no bindings
updates. Cross-parameter validation (rejecting `stream_options` with
`stream=false`) is deliberately out of P2's schema-only charter and is
being tracked as a follow-up on the codex P2 dismissal.
Verify
------
- `cargo check --workspace --tests` → clean
- `cargo test -p smg --lib` → 559 passed (3 new)
- `cargo test -p openai-protocol` → 19 passed
- `cargo clippy -p smg -p openai-protocol --lib --bins --tests -- -D warnings` → clean
- `cargo +nightly fmt --all -- --check` → clean
Refs: #1278
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cdf09afd4
ℹ️ 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".
| // Preserve caller-provided stream_options (e.g. `include_obfuscation: false` | ||
| // on the Responses API) and only default `include_usage` when the caller | ||
| // did not set it. Non-streaming requests intentionally drop stream_options. | ||
| stream_options: if is_streaming { |
There was a problem hiding this comment.
When building ChatCompletionRequest in responses_to_chat, the new ResponsesRequest fields prompt_cache_key and safety_identifier are never copied, so they always fall back to None via ..Default::default(). In the regular gRPC Responses flow, requests that set either field will deserialize successfully but the backend never receives them, so prompt-cache routing and safety attribution are silently dropped.
Useful? React with 👍 / 👎.
Contributor
|
Hi @slin1237, 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 |
mergify
Bot
added
the
needs-rebase
slin1237
added a commit
that referenced
this pull request
Apr 21, 2026
… bot feedback)
What
----
- model_gateway/src/routers/grpc/regular/responses/conversions.rs:
- `responses_to_chat` no longer overwrites `ResponsesRequest.stream_options`
with a fresh `StreamOptions { include_usage: Some(true), ..default() }`
literal. The caller's `stream_options` is now cloned forward and only
`include_usage` is defaulted to `Some(true)` when the caller did not set
it. Non-streaming requests still intentionally drop `stream_options`.
- Drop the now test-only `StreamOptions` import from the module-level use
block and re-import it inside `mod tests` so the non-test build does not
emit `unused_import`.
- Add three regression tests:
* `test_stream_options_include_obfuscation_roundtrip` — proves
`include_obfuscation: Some(false)` survives the conversion and
`include_usage` still defaults to `Some(true)` when unset.
* `test_stream_options_caller_include_usage_preserved` — proves the
caller's `include_usage: Some(false)` is not clobbered.
* `test_stream_options_non_streaming_dropped` — confirms `stream=false`
yields `stream_options: None` even when the caller set fields.
Why
---
Codex bot (P1 Major) on PR #1278 flagged that a streaming request with
`stream_options.include_obfuscation=false` was being converted to a
ChatCompletionRequest whose `include_obfuscation` silently became `None`,
discarding caller intent. This is a wire-semantics bug introduced by the P2
schema additions: adding `stream_options` to `ResponsesRequest` without
threading it through the gRPC conversion path left the field accepted but
ignored.
How
---
Switch the literal construction to
`let mut opts = req.stream_options.clone().unwrap_or_default();` followed by
an `if opts.include_usage.is_none()` backfill. This preserves every caller-
supplied field (including future `StreamOptions` additions via `Default`)
while keeping the existing `include_usage=true` behaviour internal pipeline
consumers rely on for end-of-stream usage emission.
Scope
-----
Bug-fix only; no protocol schema changes, no new CLI flags, no bindings
updates. Cross-parameter validation (rejecting `stream_options` with
`stream=false`) is deliberately out of P2's schema-only charter and is
being tracked as a follow-up on the codex P2 dismissal.
Verify
------
- `cargo check --workspace --tests` → clean
- `cargo test -p smg --lib` → 559 passed (3 new)
- `cargo test -p openai-protocol` → 19 passed
- `cargo clippy -p smg -p openai-protocol --lib --bins --tests -- -D warnings` → clean
- `cargo +nightly fmt --all -- --check` → clean
Refs: #1278
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
feat/audit-p2-top-level
branch
from
April 21, 2026 14:50
6cdf09a to
e622f89
Compare
What:
- Extend `openai_protocol::common::StreamOptions` with an optional
`include_obfuscation: Option<bool>` and derive `Default` so the struct
is now the shared Chat-and-Responses stream-options type.
- Introduce `PromptCacheRetention` (`"in-memory"` | `"24h"`),
`ContextManagementEntry { type, compact_threshold }`,
`ContextManagementType::Compaction`, and a `ConversationRef`
untagged union (`string | { id: string }`) with an `as_id()` helper
in `openai_protocol::common` for downstream tasks (P6 consumes
`ConversationRef`).
- Add six new top-level fields to `ResponsesRequest`:
`prompt: Option<ResponsePrompt>`, `prompt_cache_key: Option<String>`,
`prompt_cache_retention: Option<PromptCacheRetention>`,
`safety_identifier: Option<String>`, `stream_options: Option<StreamOptions>`,
`context_management: Option<Vec<ContextManagementEntry>>`, all
`#[serde(skip_serializing_if = "Option::is_none")]`. Extend the
`Default` impl accordingly.
- Thread the six new fields through the gRPC regular-mode tool-loop
continuation (`build_next_request` in
`model_gateway/src/routers/grpc/regular/responses/common.rs`) so
multi-turn loops preserve prompt template, cache key, safety
identifier, streaming options, and context-management config.
- Update `responses_to_chat` in
`model_gateway/src/routers/grpc/regular/responses/conversions.rs`
and two chat-completion spec tests to use
`..StreamOptions::default()` (now that the struct has a `Default`
impl) instead of field-exhaustive literals.
- Add three serde round-trip tests in `crates/protocols/src/responses.rs`
covering the full P2 field set, the `"in-memory"` retention variant,
and absent-field omission on the wire.
Why:
- The SMG protocol types were missing six top-level fields required by
the OpenAI Responses API spec (§Body Parameters: `prompt`,
`prompt_cache_key`, `prompt_cache_retention`, `safety_identifier`,
`stream_options`, `context_management`). Today spec-valid requests
carrying any of these are silently dropped during deserialization,
which masks client intent and breaks upstream routing decisions
(cache reuse, obfuscation, compaction thresholds) that depend on
these knobs. Adding them as typed `Option<_>` fields makes the
gateway forward them losslessly and gives future tasks (P6, routing
wiring) a typed surface to consume.
How:
- Grouped the new Responses-only types in `common.rs` next to the
existing `ResponsePrompt` / `PromptVariable` cluster so every
Responses-API-shared type lives in one place.
- Reused the existing `common::StreamOptions` rather than introducing
a Responses-specific duplicate: the OpenAI wire shape is identical
modulo two optional fields, and collapsing to one type avoids drift
between chat and responses streaming-options handling. `Default`
derive lets existing call sites keep their explicit
`include_usage` literal via `..StreamOptions::default()`.
- Defined `ConversationRef` now (explicitly listed in the P2 audit
entry) even though no existing field uses it yet, so P6 can migrate
`ResponsesRequest::conversation` to `Option<ConversationRef>`
without reopening this PR; kept `ResponsesRequest::conversation`
as `Option<String>` here to respect P6's dependency contract
(P6 depends on P2 for the type; P6 owns the validator and
`history.rs` persistence migration).
- Chose `ContextManagementType` as a closed enum (only `Compaction`)
rather than `String`: the spec today enumerates exactly one value,
and a closed enum aligns with P5's fail-fast-on-unknown direction
(unknown values serde-fail instead of being silently accepted).
- All `Option<_>` fields use `skip_serializing_if = "Option::is_none"`
so absent fields stay absent on the wire — verified by
`test_responses_request_new_fields_omitted_when_absent`.
Refs: P2
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Tech Lead P2 cycle-1 REJECT: ConversationRef was `pub` with zero callers — §7 "Every new pub item must be imported somewhere". P6 owns the ConversationRef type AND its migration into ResponsesRequest::conversation; pre-landing it here is speculative. Removed the enum + its impl + section comment. No change to the 6 new top-level ResponsesRequest fields or other P2 additions. Refs: P2 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
… bot feedback)
What
----
- model_gateway/src/routers/grpc/regular/responses/conversions.rs:
- `responses_to_chat` no longer overwrites `ResponsesRequest.stream_options`
with a fresh `StreamOptions { include_usage: Some(true), ..default() }`
literal. The caller's `stream_options` is now cloned forward and only
`include_usage` is defaulted to `Some(true)` when the caller did not set
it. Non-streaming requests still intentionally drop `stream_options`.
- Drop the now test-only `StreamOptions` import from the module-level use
block and re-import it inside `mod tests` so the non-test build does not
emit `unused_import`.
- Add three regression tests:
* `test_stream_options_include_obfuscation_roundtrip` — proves
`include_obfuscation: Some(false)` survives the conversion and
`include_usage` still defaults to `Some(true)` when unset.
* `test_stream_options_caller_include_usage_preserved` — proves the
caller's `include_usage: Some(false)` is not clobbered.
* `test_stream_options_non_streaming_dropped` — confirms `stream=false`
yields `stream_options: None` even when the caller set fields.
Why
---
Codex bot (P1 Major) on PR #1278 flagged that a streaming request with
`stream_options.include_obfuscation=false` was being converted to a
ChatCompletionRequest whose `include_obfuscation` silently became `None`,
discarding caller intent. This is a wire-semantics bug introduced by the P2
schema additions: adding `stream_options` to `ResponsesRequest` without
threading it through the gRPC conversion path left the field accepted but
ignored.
How
---
Switch the literal construction to
`let mut opts = req.stream_options.clone().unwrap_or_default();` followed by
an `if opts.include_usage.is_none()` backfill. This preserves every caller-
supplied field (including future `StreamOptions` additions via `Default`)
while keeping the existing `include_usage=true` behaviour internal pipeline
consumers rely on for end-of-stream usage emission.
Scope
-----
Bug-fix only; no protocol schema changes, no new CLI flags, no bindings
updates. Cross-parameter validation (rejecting `stream_options` with
`stream=false`) is deliberately out of P2's schema-only charter and is
being tracked as a follow-up on the codex P2 dismissal.
Verify
------
- `cargo check --workspace --tests` → clean
- `cargo test -p smg --lib` → 559 passed (3 new)
- `cargo test -p openai-protocol` → 19 passed
- `cargo clippy -p smg -p openai-protocol --lib --bins --tests -- -D warnings` → clean
- `cargo +nightly fmt --all -- --check` → clean
Refs: #1278
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
feat/audit-p2-top-level
branch
from
April 21, 2026 16:14
e622f89 to
4e58478
Compare
mergify
Bot
removed
the
needs-rebase
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 `@crates/protocols/src/responses.rs`:
- Around line 2013-2144: Add a negative serde test that verifies deserializing a
ResponsesRequest with context_management[0].type set to an unknown value (e.g.,
"future_mode") fails; create a new test function (e.g.,
test_context_management_unknown_type_rejects) that builds a payload with
"context_management": [{"type":"future_mode"}], attempts
serde_json::from_value::<ResponsesRequest>(...), and asserts the result is an
error (is_err() or unwrap_err()), referencing ResponsesRequest and the
ContextManagementType enum to lock in closed-enum behavior and prevent
accidental #[serde(other)] regressions.
🪄 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: 184cba37-0877-48a3-b3dc-bab28a3af427
📒 Files selected for processing (6)
crates/protocols/src/common.rscrates/protocols/src/responses.rsmodel_gateway/src/routers/grpc/regular/responses/common.rsmodel_gateway/src/routers/grpc/regular/responses/conversions.rsmodel_gateway/tests/api/responses_api_test.rsmodel_gateway/tests/spec/chat_completion.rs
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements audit task P2: add the 6 missing top-level fields to
ResponsesRequest(prompt,prompt_cache_key,prompt_cache_retention,safety_identifier,stream_options,context_management) and the supportingPromptCacheRetention/ContextManagementEntry/ContextManagementType::Compactiontypes, plus extending sharedStreamOptionswithinclude_obfuscation. Unblocks audit tasks P6 (conversation type) and I3 (compaction item).What changed
Commits on branch (
main..HEAD):98245338—feat(protocols): implement P2 top-level fields(substance)1730e5c6—refactor(protocols): drop speculative ConversationRef from P2 (cycle 2)(cycle-1 REJECT remediation — removed theConversationRefdead-pub type per §7)Diff totals (vs main): 6 files, +312 / -3.
crates/protocols/src/responses.rs(+180 / -3): 6 new top-levelResponsesRequestfields threaded into theResponsesRequeststruct + updatedDefaultimpl + 3 serde round-trip tests covering (a) spec-full-field positive roundtrip with24h+ compaction +include_obfuscation=false, (b) alternate enum variantin-memory, (c)skip_serializing_ifabsent-field verification.crates/protocols/src/common.rs(+44 / new types):PromptCacheRetentionenum (InMemory/Duration24hwith hyphen serde renamesin-memory/24h);ContextManagementEntrystruct +ContextManagementType::Compactionclosed enum (no#[serde(other)], no#[serde(untagged)]; unknown variants serde-fail, aligned with P5's future silent-swallow removal); existingStreamOptionsextended withinclude_obfuscation: Option<bool>+ derivedDefault.model_gateway/src/routers/grpc/regular/responses/common.rs(+10):build_next_requestthreads the 6 new fields through tool-loop continuations (compile-forced and semantically correct — multi-turn continuations must not silently drop the caller's prompt template / cache key / safety ID).model_gateway/src/routers/grpc/regular/responses/conversions.rs(+1):..StreamOptions::default()afterStreamOptionsgainedinclude_obfuscation(compile-forced).model_gateway/tests/api/responses_api_test.rs(+78 compile-forced): 13 struct-literal test fixtures extended withNonefor the 6 new fields (verified compile-forced by revert-test — removing oneprompt: Noneyieldserror[E0063]: missing field 'prompt' in initializer of ResponsesRequest).model_gateway/tests/spec/chat_completion.rs(+2):..StreamOptions::default()sites (compile-forced; sharedStreamOptionstype).Why
OpenAI Responses API spec defines 6 additional top-level fields on
ResponsesRequest(see.claude/_audit/openai-responses-api-spec.mdrequest-body section). smg's previous schema lacked all of them; spec-valid clients using prompt templates, cache-key hinting, safety identifiers, obfuscation options, or context-management directives hit deserialization failures. Threading through the tool-loop continuation inbuild_next_requestensures multi-turn request state carries through correctly.The closed
ContextManagementType::Compactionenum (no#[serde(other)], no#[serde(untagged)]) aligns with P5's fail-fast direction — unknown variants surface at deserialize time rather than silent-swallow.Verification
cargo check -p openai-protocolclean (isolatedCARGO_TARGET_DIR=/tmp/p2-c2-lead-target)cargo check --workspace --testscleancargo test -p openai-protocol→ 85/0 (58 unit + 8 background_mode_protocol + 18 skills_protocol + 1 doc)cargo clippy -p openai-protocol --tests -- -D warningscleancargo +nightly fmt --all -- --checksilentPromptCacheRetentionboth variants byte-identical with hyphen forms +ContextManagementType::Compactionclosed-enum rejects unknown +StreamOptions.include_obfuscationoptional absent-vs-present)ConversationRefdeletionerror[E0063])ContextManagementTypeclosed-enum claim verified: no#[serde(other)], no#[serde(untagged)],rename_all = "snake_case"only (value renaming, not a catch-all)StreamOptionsreuse safety: Go SDK (bindings/golang/client.go:203) and Python SDK (clients/python/smg_client/types/__init__.py) mirrors only reference the type;include_obfuscationisOption<bool>withskip_serializing_if— absent on wire for non-setters, no chat-path breakageResponsesRequest::conversation: Option<String>intact atresponses.rs:703(field type and identity unchanged — P6 owns the migration)unavailable(harness skill permission; Lead proceeded solo per playbook §8 fallback after own 19 fixtures passed both cycles)Blast radius
6 files. Protocol schema (2) + router propagation (2) + test-fixture compile-forced extensions (2). Matches audit Blast Radius; no forbidden files touched.
Out of scope
ResponsesRequest::conversationtype (currentlyOption<String>) — P6 owns the migration to a string-or-objectConversationRefunion.build_next_request(needed for correctness) but not upstream.Follow-ups noted (non-blocking)
ConversationRefdead-pub landed in cycle 1, removed in cycle 2 per Lead cycle-1 REJECT. P6 will re-introduce when it consumes the type.Refs: audit task P2 ·
.claude/_audit/responses-api-gap-audit.mdSummary by CodeRabbit
New Features
Tests