fix(mcp): hide internal tools in grpc responses#1412
Conversation
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
zhoug9127
requested review from
CatherineSue,
key4ng and
slin1237
as code owners
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
Bot
added
grpc
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed all 6 changed files. The three-layer redaction approach (streaming events, response.completed event, persisted final response) is well-structured and handles edge cases correctly:
- Hidden tool calls are excluded from client-facing stream events but still executed and fed back into the model's conversation state
tool_call_trackinglookups safely skip missing entries for hidden toolsuser_function_namesprevents false-positive hiding of user-defined function tools that share names with internal MCP toolstool_choicereferencing hidden tools is correctly reset to"auto"mcp_list_toolsemission is skipped for internal servers in both streaming paths- Non-Passthrough response formats (e.g., ImageGenerationCall) are correctly kept visible
No issues found — 0 🔴, 0 🟡, 0 🟣.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/grpc/harmony/responses/streaming.rs`:
- Around line 182-184: The current check only skips non-builtin internal server
labels (session.is_internal_non_builtin_server_label(&binding.label)) so
builtin-routed internal labels still get through mcp_list_tools; change the
condition to suppress all internal server labels by using a broader predicate
(e.g., session.is_internal_server_label(&binding.label)) or combine predicates
(session.is_internal_non_builtin_server_label(...) ||
session.is_internal_builtin_server_label(...)) so any internal server label with
binding.label is filtered out from mcp_list_tools output.
In `@model_gateway/src/routers/grpc/regular/responses/streaming.rs`:
- Around line 561-563: The current filter uses
session.is_internal_non_builtin_server_label(&binding.label) which only hides
mcp_list_tools for non-builtin internal servers; change the check to cover all
internal server labels (e.g., session.is_internal_server_label(&binding.label))
or otherwise reuse McpToolSession::should_hide_output_item_json logic to decide
hiding so builtin-routed internal servers are also filtered and do not emit
mcp_list_tools; update the condition at the binding label check to use that
broader/internal check and ensure behavior matches
McpToolSession::should_hide_output_item_json.
🪄 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: ac89afce-92d9-4842-afeb-0c285ab9a120
📒 Files selected for processing (6)
model_gateway/src/routers/grpc/common/responses/utils.rsmodel_gateway/src/routers/grpc/harmony/responses/non_streaming.rsmodel_gateway/src/routers/grpc/harmony/responses/streaming.rsmodel_gateway/src/routers/grpc/harmony/streaming.rsmodel_gateway/src/routers/grpc/regular/responses/non_streaming.rsmodel_gateway/src/routers/grpc/regular/responses/streaming.rs
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_gateway/src/routers/grpc/regular/responses/streaming.rs (1)
231-232: 🧹 Nitpick | 🔵 TrivialConsider adding redaction for consistency in non-MCP streaming path.
Similar to the harmony file, the non-MCP path (
process_and_transform_sse_stream) does not applyredact_response_completed_eventto the completion event. While this path has no MCP session (so redaction would likely be a no-op), adding the call would provide consistency and defense-in-depth.♻️ Optional: Add redaction call for consistency
- let completed_event = event_emitter.emit_completed(usage_json.as_ref()); + let mut completed_event = event_emitter.emit_completed(usage_json.as_ref()); + redact_response_completed_event(&mut completed_event, &original_request, None); event_emitter.send_event(&completed_event, &tx)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/responses/streaming.rs` around lines 231 - 232, The completion event emitted in process_and_transform_sse_stream currently skips redaction; to add consistency and defense-in-depth, call redact_response_completed_event before sending: obtain the completed_event from event_emitter.emit_completed(usage_json.as_ref()), pass it through redact_response_completed_event (preserving any return type), then call event_emitter.send_event(&redacted_completed_event, &tx)?; update imports/signature if needed to accept the redacted event type and ensure usage_json and tx are passed through unchanged.model_gateway/src/routers/grpc/harmony/responses/streaming.rs (1)
489-514: 🧹 Nitpick | 🔵 TrivialConsider adding redaction for consistency in non-MCP path.
The non-MCP path (
execute_without_mcp_streaming) does not apply redaction tofinal_response(line 489) or theresponse.completedevent (line 513), while the MCP path does. Although redaction is likely a no-op whensession=None(based onClientRedaction::newguard), calling the redaction helpers here would provide defense-in-depth if any edge case surfaces internal tool artifacts in non-MCP flows.This is a minor consistency concern rather than a functional gap given the current architecture.
♻️ Optional: Add redaction calls for consistency
// Finalize response from emitter's accumulated data - let final_response = emitter.finalize(Some(usage.clone())); + let mut final_response = emitter.finalize(Some(usage.clone())); + redact_response_for_client(&mut final_response, original_request, None); // Persist response to storage if store=true // ... - let event = emitter.emit_completed(Some(&usage_json)); + let mut event = emitter.emit_completed(Some(&usage_json)); + redact_response_completed_event(&mut event, original_request, None); emitter.send_event_best_effort(&event, tx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/harmony/responses/streaming.rs` around lines 489 - 514, Add the same redaction step used in the MCP path: create a ClientRedaction via ClientRedaction::new(session) (or equivalent redactor) in execute_without_mcp_streaming, run the redactor on final_response before calling persist_response_if_needed, and run the redactor on the completed event (the value returned by emitter.emit_completed) before emitter.send_event_best_effort; use the same redaction helper methods used elsewhere (e.g., redactor.redact_response(...) and redactor.redact_event(...) or their equivalents) so non‑MCP flows get the same defense-in-depth redaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/routers/grpc/harmony/responses/streaming.rs`:
- Around line 489-514: Add the same redaction step used in the MCP path: create
a ClientRedaction via ClientRedaction::new(session) (or equivalent redactor) in
execute_without_mcp_streaming, run the redactor on final_response before calling
persist_response_if_needed, and run the redactor on the completed event (the
value returned by emitter.emit_completed) before emitter.send_event_best_effort;
use the same redaction helper methods used elsewhere (e.g.,
redactor.redact_response(...) and redactor.redact_event(...) or their
equivalents) so non‑MCP flows get the same defense-in-depth redaction.
In `@model_gateway/src/routers/grpc/regular/responses/streaming.rs`:
- Around line 231-232: The completion event emitted in
process_and_transform_sse_stream currently skips redaction; to add consistency
and defense-in-depth, call redact_response_completed_event before sending:
obtain the completed_event from
event_emitter.emit_completed(usage_json.as_ref()), pass it through
redact_response_completed_event (preserving any return type), then call
event_emitter.send_event(&redacted_completed_event, &tx)?; update
imports/signature if needed to accept the redacted event type and ensure
usage_json and tx are passed through unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cade082-ed5a-4804-a0de-3cdad04843be
📒 Files selected for processing (2)
model_gateway/src/routers/grpc/harmony/responses/streaming.rsmodel_gateway/src/routers/grpc/regular/responses/streaming.rs
|
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! |
1 participant
Description
Problem
#1405 hides internal MCP details from OpenAI Responses streaming/final output. The gRPC regular and harmony Responses paths also need the same client-facing behavior: internal non-builtin MCP tools may participate in the agent loop, but their tool lists, calls, arguments, outputs, labels, and tool choices must not appear in customer-visible gRPC responses or streams.
Solution
Add shared gRPC response redaction helpers and wire them through regular and harmony Responses paths. Streaming now suppresses internal non-builtin MCP tool events while preserving public/customer tools and hosted output formats such as
image_generation_call.This PR is stacked on #1405 and targets
fix/hide-internal-mcp-streaming.Changes
image_generation_call.Test Plan
cargo test -p smg grpc_response_redaction_strips_internal_mcp_artifacts --all-featurescargo test -p smg grpc_streaming_visibility_keeps_public_and_hosted_formats_visible --all-featurespre-commit run --all-filesChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit