fix(openai-bridge): address PR #1429 review follow-ups (alias miss, error state, wire shape)

crates/mcp/src/core/orchestrator.rs

@@ -158,7 +158,8 @@ pub struct ToolExecutionInput {
 /// Output from batch tool execution.
 ///
 /// `#[non_exhaustive]` so additive fields don't break consumers; only
-/// `smg-mcp` constructs this.
+/// `smg-mcp` constructs this in production. External crates that need to
+/// fabricate one for tests should use [`ToolExecutionOutput::new_for_test`].
 #[derive(Debug, Clone)]
 #[non_exhaustive]
 pub struct ToolExecutionOutput {
@@ -184,6 +185,45 @@ pub struct ToolExecutionOutput {
     pub duration: Duration,
 }
 
+impl ToolExecutionOutput {
+    /// Construct an instance from explicit field values.
+    ///
+    /// `#[non_exhaustive]` blocks struct-literal syntax in downstream crates,
+    /// but those crates still need a way to fabricate fixtures (e.g. router
+    /// transformer tests that need a `is_error: true` output without spinning
+    /// up an MCP server). Adding a field to the struct doesn't break this
+    /// constructor — only adding a *required* field would, which is a
+    /// deliberate signal that all callers need to think about the new field.
+    #[expect(
+        clippy::too_many_arguments,
+        reason = "Test fixture constructor; one positional arg per public field is intentional \
+                  so adding a field forces every test to consider it."
+    )]
+    pub fn new_for_test(
+        call_id: impl Into<String>,
+        tool_name: impl Into<String>,
+        server_key: impl Into<String>,
+        server_label: impl Into<String>,
+        arguments_str: impl Into<String>,
+        output: Value,
+        is_error: bool,
+        error_message: Option<String>,
+        duration: Duration,
+    ) -> Self {
+        Self {
+            call_id: call_id.into(),
+            tool_name: tool_name.into(),
+            server_key: server_key.into(),
+            server_label: server_label.into(),
+            arguments_str: arguments_str.into(),
+            output,
+            is_error,
+            error_message,
+            duration,
+        }
+    }
+}
+
 /// Result from resolved tool execution that preserves interactive approval state.
 #[derive(Debug, Clone)]
 pub enum ToolExecutionResult {

model_gateway/src/routers/common/mcp_utils.rs

@@ -247,6 +247,22 @@ pub fn extract_builtin_types(tools: &[ResponseTool]) -> Vec<BuiltinToolType> {
         .collect()
 }
 
+/// True if `tools` carries any MCP-routed entry (declared MCP server or a
+/// builtin family that the gateway intercepts via MCP). Used by the OpenAI
+/// router to decide whether the format-registry component is required for
+/// the request — plain-tool requests don't need it and must not 500.
+pub fn request_uses_mcp_routing(tools: &[ResponseTool]) -> bool {
+    tools.iter().any(|t| {
+        matches!(
+            t,
+            ResponseTool::Mcp(_)
+                | ResponseTool::WebSearchPreview(_)
+                | ResponseTool::CodeInterpreter(_)
+                | ResponseTool::ImageGeneration(_)
+        )
+    })
+}
+
 /// Collect user-declared function tool names from a Responses request.
 pub(crate) fn collect_user_function_names(request: &ResponsesRequest) -> HashSet<String> {
     request

model_gateway/src/routers/common/openai_bridge/format_registry.rs

@@ -52,54 +52,84 @@ impl FormatRegistry {
         self.formats.insert(qualified, format);
     }
 
+    fn remove(&self, qualified: &QualifiedToolName) {
+        self.formats.remove(qualified);
+    }
+
     /// Populate from a server config: per-tool overrides + builtin defaults.
-    /// Safe to call repeatedly — entries for non-Passthrough formats are
-    /// overwritten. Downgrading a format back to `Passthrough` requires a
-    /// separate registry rebuild (no production caller mutates configs in
-    /// place today).
     ///
-    /// Mirrors `McpOrchestrator::apply_tool_configs`:
-    /// - When a tool has an `alias`, the format is attached **only** to the
-    ///   alias entry (under `("alias", alias_name)`), matching the orchestrator's
-    ///   `register_alias` qualified-name shape. The underlying `(server, tool)`
-    ///   stays at the `Passthrough` default so direct calls aren't transformed.
-    /// - When a tool has no alias but a non-default format, attach to
-    ///   `(server, tool)` directly.
-    /// - When the per-tool stanza omits `response_format` entirely
-    ///   (`None`), the builtin default still applies. This lets users add an
-    ///   `alias` or `arg_mapping` to a builtin tool without disabling its
-    ///   hosted-format wire shape. An explicit `Some(Passthrough)` *does*
-    ///   block the builtin default — that is the documented escape hatch
-    ///   for opting out of the hosted shape.
+    /// Safe to call repeatedly. Each affected key is unconditionally rewritten
+    /// (or removed for an explicit `Some(Passthrough)` downgrade) so a later
+    /// config that demotes a tool from a hosted format back to `Passthrough`
+    /// does not leave the previous entry behind in the map.
+    ///
+    /// Mirrors `McpOrchestrator::apply_tool_configs` *and* the dispatch shape
+    /// of `McpToolSession`. Two production lookup paths must resolve to the
+    /// same format:
+    /// - via the alias key `("alias", alias_name)` — what the session exposes
+    ///   when `collect_visible_mcp_tools` replaces the direct entry with its
+    ///   alias entry (see `crates/mcp/src/core/session.rs:565-571`).
+    /// - via the underlying `(server_key, tool_name)` — what direct dispatch
+    ///   uses when no alias hides the tool.
+    ///
+    /// So when a tool has an alias the format is mirrored on **both** keys.
+    /// Aliased builtin tools get the same treatment, and only when the
+    /// per-tool stanza omits `response_format` entirely (`None`) is the
+    /// builtin default applied; an explicit `Some(Passthrough)` is the
+    /// documented escape hatch for opting out of the hosted shape.
     pub fn populate_from_server_config(&self, config: &McpServerConfig) {
         if let Some(tools) = &config.tools {
             for (tool_name, tool_config) in tools {
+                let direct_key = QualifiedToolName::new(&config.name, tool_name);
+                let alias_key = tool_config
+                    .alias
+                    .as_deref()
+                    .map(|alias| QualifiedToolName::new(ALIAS_SERVER_KEY, alias));
+
                 let Some(format_config) = tool_config.response_format else {
+                    // `response_format` omitted: defer to the builtin-default
+                    // pass below. Don't touch existing entries here so a
+                    // direct-dispatch override placed by an earlier loop
+                    // iteration survives.
                     continue;
                 };
                 let format: ResponseFormat = format_config.into();
                 if format == ResponseFormat::Passthrough {
+                    // Explicit downgrade — clear any prior hosted-format entry
+                    // on every key the production lookup might consult.
+                    self.remove(&direct_key);
+                    if let Some(alias_key) = &alias_key {
+                        self.remove(alias_key);
+                    }
                     continue;
                 }
-                if let Some(alias) = &tool_config.alias {
-                    self.insert(QualifiedToolName::new(ALIAS_SERVER_KEY, alias), format);
-                } else {
-                    self.insert(QualifiedToolName::new(&config.name, tool_name), format);
+
+                // Mirror the format on every key production might query.
+                // Also write the direct key so that direct dispatch (which
+                // does not go through the alias rewrite) still gets the
+                // hosted shape.
+                self.insert(direct_key, format);
+                if let Some(alias_key) = alias_key {
+                    self.insert(alias_key, format);
                 }
             }
         }
 
         if let (Some(builtin_type), Some(tool_name)) =
             (&config.builtin_type, &config.builtin_tool_name)
         {
-            let has_explicit_format = config
-                .tools
-                .as_ref()
-                .and_then(|tools| tools.get(tool_name))
-                .is_some_and(|cfg| cfg.response_format.is_some());
+            let stanza = config.tools.as_ref().and_then(|tools| tools.get(tool_name));
+            let has_explicit_format = stanza.is_some_and(|cfg| cfg.response_format.is_some());
             if !has_explicit_format {
                 let format: ResponseFormat = builtin_type.response_format().into();
                 self.insert(QualifiedToolName::new(&config.name, tool_name), format);
+                // `collect_visible_mcp_tools` exposes the alias entry instead
+                // of the direct entry, so the production lookup queries
+                // `("alias", alias)`. Without this mirror an alias-only stanza
+                // on a builtin tool silently degrades to `Passthrough`.
+                if let Some(alias) = stanza.and_then(|cfg| cfg.alias.as_deref()) {
+                    self.insert(QualifiedToolName::new(ALIAS_SERVER_KEY, alias), format);
+                }
             }
         }
     }
@@ -142,9 +172,11 @@ mod tests {
     }
 
     #[test]
-    fn alias_format_stored_under_alias_server_key() {
-        // Mirrors orchestrator::register_alias which uses
-        // QualifiedToolName::new("alias", alias_name).
+    fn alias_format_mirrored_on_both_keys() {
+        // The session lookup path goes through `("alias", alias_name)` because
+        // `collect_visible_mcp_tools` replaces the direct entry with its alias
+        // entry. Direct dispatch (no alias rewrite) still queries
+        // `(server_key, tool_name)`, so both keys must carry the format.
         let mut tools = HashMap::new();
         tools.insert(
             "brave_web_search".to_string(),
@@ -167,8 +199,9 @@ mod tests {
         );
         assert_eq!(
             r.lookup_by_names("brave", "brave_web_search"),
-            ResponseFormat::Passthrough,
-            "underlying tool entry must NOT receive the format when an alias exists"
+            ResponseFormat::WebSearchCall,
+            "direct (server_key, tool_name) entry must also carry the format \
+             so direct dispatch resolves the same shape as alias dispatch"
         );
     }
 
@@ -239,12 +272,15 @@ mod tests {
     }
 
     #[test]
-    fn alias_only_stanza_preserves_builtin_default() {
+    fn alias_only_stanza_preserves_builtin_default_on_both_keys() {
         // Regression: a per-tool stanza that only aliases a builtin tool
         // (or only sets arg_mapping) used to suppress the builtin default,
-        // collapsing the hosted format to plain mcp_call. With
-        // `response_format: None` meaning "inherit context", the builtin
-        // default must still apply.
+        // collapsing the hosted format to plain mcp_call. The crucial
+        // production path is the alias key — `collect_visible_mcp_tools`
+        // exposes the alias entry, so `lookup_tool_format(session, …,
+        // "web_search")` resolves to `("alias", "web_search")`. If the
+        // builtin default only landed at the direct key, dispatch silently
+        // degrades to `Passthrough`.
         let mut tools = HashMap::new();
         tools.insert(
             "do_search".to_string(),
@@ -262,10 +298,74 @@ mod tests {
         let r = FormatRegistry::new();
         r.populate_from_server_config(&cfg);
 
+        // Alias key — what production session lookup actually hits.
+        assert_eq!(
+            r.lookup_by_names("alias", "web_search"),
+            ResponseFormat::WebSearchCall,
+            "alias-only stanza on a builtin must mirror the hosted format on \
+             the alias key — that is the key production resolves through"
+        );
+        // Direct key — what direct dispatch hits.
         assert_eq!(
             r.lookup_by_names("search", "do_search"),
             ResponseFormat::WebSearchCall,
-            "alias-only stanza must not disable the builtin's hosted format"
+            "direct (server, tool) lookup must still resolve the hosted format"
+        );
+    }
+
+    #[test]
+    fn explicit_passthrough_downgrade_clears_prior_hosted_entry() {
+        // `populate_from_server_config` is called every time a server
+        // (re)registers, so a config that flips a tool from a hosted format
+        // back to `Passthrough` must clear the stale entry — otherwise the
+        // registry keeps transforming outputs as the old hosted type.
+        let r = FormatRegistry::new();
+
+        let mut hosted = HashMap::new();
+        hosted.insert(
+            "brave_web_search".to_string(),
+            ToolConfig {
+                alias: Some("web_search".to_string()),
+                response_format: Some(ResponseFormatConfig::WebSearchCall),
+                arg_mapping: None,
+            },
+        );
+        let mut hosted_cfg = server("brave");
+        hosted_cfg.tools = Some(hosted);
+        r.populate_from_server_config(&hosted_cfg);
+        assert_eq!(
+            r.lookup_by_names("alias", "web_search"),
+            ResponseFormat::WebSearchCall,
+            "precondition: alias key carries the hosted format"
+        );
+        assert_eq!(
+            r.lookup_by_names("brave", "brave_web_search"),
+            ResponseFormat::WebSearchCall,
+            "precondition: direct key carries the hosted format"
+        );
+
+        let mut downgraded = HashMap::new();
+        downgraded.insert(
+            "brave_web_search".to_string(),
+            ToolConfig {
+                alias: Some("web_search".to_string()),
+                response_format: Some(ResponseFormatConfig::Passthrough),
+                arg_mapping: None,
+            },
+        );
+        let mut downgraded_cfg = server("brave");
+        downgraded_cfg.tools = Some(downgraded);
+        r.populate_from_server_config(&downgraded_cfg);
+
+        assert_eq!(
+            r.lookup_by_names("alias", "web_search"),
+            ResponseFormat::Passthrough,
+            "explicit Passthrough must clear the previous alias entry"
+        );
+        assert_eq!(
+            r.lookup_by_names("brave", "brave_web_search"),
+            ResponseFormat::Passthrough,
+            "explicit Passthrough must clear the previous direct entry"
         );
     }
 }

model_gateway/src/routers/common/openai_bridge/tool_descriptors.rs

@@ -85,18 +85,24 @@ pub fn response_tools(session: &McpToolSession<'_>) -> Vec<ResponseTool> {
 }
 
 /// `McpToolInfo` records used inside `mcp_list_tools` output items.
+///
+/// Wire-shape note: OpenAI's Responses API exposes only `{"read_only": …}`
+/// inside `mcp_list_tools[].tools[].annotations`. The richer SMG
+/// `ToolAnnotations` (which also carries `destructive`/`idempotent`/
+/// `open_world`) is intentionally narrowed here to match that shape — those
+/// extra hints are read directly off `ToolEntry::annotations` by the approval
+/// pipeline and would only confuse downstream OpenAI-compatible clients if
+/// emitted on the wire.
 pub fn build_mcp_tool_infos(entries: &[smg_mcp::ToolEntry]) -> Vec<McpToolInfo> {
     entries
         .iter()
         .map(|entry| McpToolInfo {
             name: entry.tool_name().to_string(),
             description: entry.tool.description.as_ref().map(|d| d.to_string()),
             input_schema: schema_to_value(&entry.tool.input_schema),
-            annotations: entry
-                .tool
-                .annotations
-                .as_ref()
-                .and_then(|a| serde_json::to_value(a).ok()),
+            annotations: Some(json!({
+                "read_only": entry.annotations.read_only,
+            })),
         })
         .collect()
 }
@@ -311,3 +317,48 @@ fn is_client_visible_output_item(
         | ResponseOutputItem::LocalShellCallOutput { .. } => true,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::{borrow::Cow, sync::Arc};
+
+    use rmcp::model::Tool;
+    use smg_mcp::{ToolAnnotations, ToolEntry};
+
+    use super::*;
+
+    fn entry_with_annotations(annotations: ToolAnnotations) -> ToolEntry {
+        let tool = Tool {
+            name: Cow::Owned("widget".to_string()),
+            title: None,
+            description: Some(Cow::Owned("widget description".to_string())),
+            input_schema: Arc::new(serde_json::Map::new()),
+            output_schema: None,
+            annotations: None,
+            icons: None,
+        };
+        ToolEntry::from_server_tool("srv", tool).with_annotations(annotations)
+    }
+
+    #[test]
+    fn build_mcp_tool_infos_emits_only_read_only_annotation() {
+        // Wire-spec parity: OpenAI's `mcp_list_tools[].tools[].annotations`
+        // exposes `{"read_only": …}` only. The richer SMG `ToolAnnotations`
+        // carries `destructive`/`idempotent`/`open_world` that the approval
+        // pipeline reads internally; surfacing them on the wire would change
+        // the documented shape.
+        let annotations = ToolAnnotations::default()
+            .with_read_only(true)
+            .with_destructive(true);
+        let entries = vec![entry_with_annotations(annotations)];
+
+        let infos = build_mcp_tool_infos(&entries);
+        assert_eq!(infos.len(), 1);
+        let serialized = serde_json::to_value(&infos[0])
+            .expect("McpToolInfo must serialize")
+            .get("annotations")
+            .cloned()
+            .expect("annotations must serialize");
+        assert_eq!(serialized, json!({ "read_only": true }));
+    }
+}