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 tests use
+/// [`ToolExecutionOutput::new_for_test`].
 #[derive(Debug, Clone)]
 #[non_exhaustive]
 pub struct ToolExecutionOutput {
@@ -184,6 +185,39 @@ pub struct ToolExecutionOutput {
     pub duration: Duration,
 }
 
+impl ToolExecutionOutput {
+    /// Test-only constructor for downstream crates blocked by
+    /// `#[non_exhaustive]`. One positional arg per public field is
+    /// intentional so adding a field forces every test to consider it.
+    #[expect(
+        clippy::too_many_arguments,
+        reason = "Test fixture constructor; one arg per field is intentional."
+    )]
+    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,17 @@ 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).
+///
+/// Derived from [`extract_builtin_types`] so the predicate and the actual
+/// routing path can't drift — adding a new builtin to the routing path
+/// (e.g. `file_search`) makes this predicate cover it too.
+pub fn request_uses_mcp_routing(tools: &[ResponseTool]) -> bool {
+    tools.iter().any(|t| matches!(t, ResponseTool::Mcp(_)))
+        || !extract_builtin_types(tools).is_empty()
+}
+
 /// 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,56 @@ impl FormatRegistry {
         self.formats.insert(qualified, format);
     }
 
-    /// 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).
+    fn remove(&self, qualified: &QualifiedToolName) {
+        self.formats.remove(qualified);
+    }
+
+    /// Populate from a server config. Safe to call repeatedly.
     ///
-    /// 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.
+    /// `McpToolSession::collect_visible_mcp_tools` replaces a direct tool
+    /// entry with its alias entry, so production session lookup of an
+    /// aliased tool resolves through `("alias", alias_name)`. Direct
+    /// dispatch still uses `(server_key, tool_name)`. Both keys must carry
+    /// the same format, so non-Passthrough formats are mirrored on both.
     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 {
                     continue;
                 };
                 let format: ResponseFormat = format_config.into();
                 if format == ResponseFormat::Passthrough {
+                    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);
+
+                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);
+                if let Some(alias) = stanza.and_then(|cfg| cfg.alias.as_deref()) {
+                    self.insert(QualifiedToolName::new(ALIAS_SERVER_KEY, alias), format);
+                }
             }
         }
     }
@@ -142,9 +144,7 @@ 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() {
         let mut tools = HashMap::new();
         tools.insert(
             "brave_web_search".to_string(),
@@ -163,12 +163,10 @@ mod tests {
         assert_eq!(
             r.lookup_by_names("alias", "web_search"),
             ResponseFormat::WebSearchCall,
-            "alias entry must use the literal `alias` server_key prefix"
         );
         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,
         );
     }
 
@@ -217,7 +215,6 @@ mod tests {
             "do_search".to_string(),
             ToolConfig {
                 alias: None,
-                // Explicit override differs from the builtin default.
                 response_format: Some(ResponseFormatConfig::Passthrough),
                 arg_mapping: None,
             },
@@ -230,21 +227,14 @@ mod tests {
         let r = FormatRegistry::new();
         r.populate_from_server_config(&cfg);
 
-        // Explicit Some(Passthrough) override means "no entry inserted" AND
-        // the builtin default is NOT applied on top.
         assert_eq!(
             r.lookup_by_names("search", "do_search"),
             ResponseFormat::Passthrough
         );
     }
 
     #[test]
-    fn alias_only_stanza_preserves_builtin_default() {
-        // 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.
+    fn alias_only_stanza_preserves_builtin_default_on_both_keys() {
         let mut tools = HashMap::new();
         tools.insert(
             "do_search".to_string(),
@@ -262,10 +252,63 @@ mod tests {
         let r = FormatRegistry::new();
         r.populate_from_server_config(&cfg);
 
+        // Alias key — what production session lookup hits.
+        assert_eq!(
+            r.lookup_by_names("alias", "web_search"),
+            ResponseFormat::WebSearchCall,
+        );
+        // 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"
+        );
+    }
+
+    #[test]
+    fn explicit_passthrough_downgrade_clears_prior_hosted_entry() {
+        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,
+        );
+        assert_eq!(
+            r.lookup_by_names("brave", "brave_web_search"),
+            ResponseFormat::WebSearchCall,
+        );
+
+        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,
+        );
+        assert_eq!(
+            r.lookup_by_names("brave", "brave_web_search"),
+            ResponseFormat::Passthrough,
         );
     }
 }

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

@@ -85,18 +85,30 @@ pub fn response_tools(session: &McpToolSession<'_>) -> Vec<ResponseTool> {
 }
 
 /// `McpToolInfo` records used inside `mcp_list_tools` output items.
+///
+/// `annotations` is narrowed to `{"read_only": …}` to match OpenAI's
+/// Responses API shape. The hint is read straight off the rmcp tool
+/// (`tool.annotations.read_only_hint`) rather than the SMG
+/// `ToolAnnotations` wrapper — the wrapper applies conservative policy
+/// defaults (destructive=true on absent hint) that are intended for the
+/// approval pipeline, not the wire surface, and reading them here would
+/// surface the wrong `read_only` for tools the server didn't annotate.
 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
+        .map(|entry| {
+            let read_only = entry
                 .tool
                 .annotations
                 .as_ref()
-                .and_then(|a| serde_json::to_value(a).ok()),
+                .and_then(|a| a.read_only_hint)
+                .unwrap_or(false);
+            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: Some(json!({ "read_only": read_only })),
+            }
         })
         .collect()
 }
@@ -311,3 +323,60 @@ fn is_client_visible_output_item(
         | ResponseOutputItem::LocalShellCallOutput { .. } => true,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::{borrow::Cow, sync::Arc};
+
+    use rmcp::model::{Tool, ToolAnnotations as RmcpToolAnnotations};
+    use smg_mcp::ToolEntry;
+
+    use super::*;
+
+    fn entry_with_rmcp_annotations(annotations: Option<RmcpToolAnnotations>) -> 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,
+            icons: None,
+        };
+        ToolEntry::from_server_tool("srv", tool)
+    }
+
+    fn read_only_hint(value: bool) -> RmcpToolAnnotations {
+        RmcpToolAnnotations {
+            title: None,
+            read_only_hint: Some(value),
+            destructive_hint: None,
+            idempotent_hint: None,
+            open_world_hint: None,
+        }
+    }
+
+    #[test]
+    fn build_mcp_tool_infos_surfaces_rmcp_read_only_hint() {
+        let entries = vec![entry_with_rmcp_annotations(Some(read_only_hint(true)))];
+        let infos = build_mcp_tool_infos(&entries);
+        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 }));
+    }
+
+    #[test]
+    fn build_mcp_tool_infos_defaults_to_false_when_hint_absent() {
+        let entries = vec![entry_with_rmcp_annotations(None)];
+        let infos = build_mcp_tool_infos(&entries);
+        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": false }));
+    }
+}