refactor(openai-bridge): finish descriptor pattern, fix FileSearch routing gap

model_gateway/src/routers/common/mcp_utils.rs

@@ -11,7 +11,8 @@ use smg_mcp::{BuiltinToolType, McpOrchestrator, McpServerBinding, McpServerConfi
 use tracing::{debug, warn};
 
 use crate::routers::common::openai_bridge::{
-    apply_hosted_tool_overrides, extract_hosted_tool_overrides, FormatRegistry, ResponseFormat,
+    self, apply_hosted_tool_overrides, extract_hosted_tool_overrides, FormatRegistry,
+    ResponseFormat,
 };
 
 /// Default maximum tool loop iterations (safety limit).
@@ -194,11 +195,8 @@ pub fn collect_builtin_routing(
     let mut routing = Vec::new();
 
     for tool in tools {
-        let builtin_type = match tool {
-            ResponseTool::WebSearchPreview(_) => BuiltinToolType::WebSearchPreview,
-            ResponseTool::CodeInterpreter(_) => BuiltinToolType::CodeInterpreter,
-            ResponseTool::ImageGeneration(_) => BuiltinToolType::ImageGeneration,
-            _ => continue,
+        let Some(builtin_type) = openai_bridge::builtin_type_for_response_tool(tool) else {
+            continue;
         };
 
         if let Some((server_name, tool_name)) = mcp_orchestrator.find_builtin_server(builtin_type) {
@@ -238,12 +236,7 @@ pub fn collect_builtin_routing(
 pub fn extract_builtin_types(tools: &[ResponseTool]) -> Vec<BuiltinToolType> {
     tools
         .iter()
-        .filter_map(|t| match t {
-            ResponseTool::WebSearchPreview(_) => Some(BuiltinToolType::WebSearchPreview),
-            ResponseTool::CodeInterpreter(_) => Some(BuiltinToolType::CodeInterpreter),
-            ResponseTool::ImageGeneration(_) => Some(BuiltinToolType::ImageGeneration),
-            _ => None,
-        })
+        .filter_map(openai_bridge::builtin_type_for_response_tool)
         .collect()
 }
 

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

@@ -14,6 +14,10 @@ use super::ResponseFormat;
 #[derive(Debug, Clone, Copy)]
 pub struct FormatDescriptor {
     pub type_str: &'static str,
+    /// Item-id kind without trailing `_` (e.g. `"ws"`, `"mcp"`). Joined to a
+    /// suffix at construction sites: `format!("{kind}_{rest}")`. Stored
+    /// without the underscore so callers that just need the discriminator
+    /// (allocator prefix, log labels) don't have to trim.
     pub id_prefix: &'static str,
     pub in_progress_event: &'static str,
     /// `None` for formats with no intermediate phase (e.g. Passthrough).
@@ -24,11 +28,27 @@ pub struct FormatDescriptor {
     pub partial_image_event: Option<&'static str>,
 }
 
+/// Reverse lookup for routers that have already received an item-type
+/// string (`mcp_call`, `web_search_call`, …) from the upstream wire and
+/// need to recover the matching `ResponseFormat` to consult the
+/// descriptor. Returns `None` for non-format item types like
+/// `function_call` or `message`.
+pub fn format_from_type_str(type_str: &str) -> Option<ResponseFormat> {
+    match type_str {
+        ItemType::MCP_CALL => Some(ResponseFormat::Passthrough),
+        ItemType::WEB_SEARCH_CALL => Some(ResponseFormat::WebSearchCall),
+        ItemType::CODE_INTERPRETER_CALL => Some(ResponseFormat::CodeInterpreterCall),
+        ItemType::FILE_SEARCH_CALL => Some(ResponseFormat::FileSearchCall),
+        ItemType::IMAGE_GENERATION_CALL => Some(ResponseFormat::ImageGenerationCall),
+        _ => None,
+    }
+}
+
 pub const fn descriptor(format: ResponseFormat) -> FormatDescriptor {
     match format {
         ResponseFormat::WebSearchCall => FormatDescriptor {
             type_str: ItemType::WEB_SEARCH_CALL,
-            id_prefix: "ws_",
+            id_prefix: "ws",
             in_progress_event: WebSearchCallEvent::IN_PROGRESS,
             searching_event: Some(WebSearchCallEvent::SEARCHING),
             completed_event: WebSearchCallEvent::COMPLETED,
@@ -37,7 +57,7 @@ pub const fn descriptor(format: ResponseFormat) -> FormatDescriptor {
         },
         ResponseFormat::CodeInterpreterCall => FormatDescriptor {
             type_str: ItemType::CODE_INTERPRETER_CALL,
-            id_prefix: "ci_",
+            id_prefix: "ci",
             in_progress_event: CodeInterpreterCallEvent::IN_PROGRESS,
             searching_event: Some(CodeInterpreterCallEvent::INTERPRETING),
             completed_event: CodeInterpreterCallEvent::COMPLETED,
@@ -46,7 +66,7 @@ pub const fn descriptor(format: ResponseFormat) -> FormatDescriptor {
         },
         ResponseFormat::FileSearchCall => FormatDescriptor {
             type_str: ItemType::FILE_SEARCH_CALL,
-            id_prefix: "fs_",
+            id_prefix: "fs",
             in_progress_event: FileSearchCallEvent::IN_PROGRESS,
             searching_event: Some(FileSearchCallEvent::SEARCHING),
             completed_event: FileSearchCallEvent::COMPLETED,
@@ -55,7 +75,7 @@ pub const fn descriptor(format: ResponseFormat) -> FormatDescriptor {
         },
         ResponseFormat::ImageGenerationCall => FormatDescriptor {
             type_str: ItemType::IMAGE_GENERATION_CALL,
-            id_prefix: "ig_",
+            id_prefix: "ig",
             in_progress_event: ImageGenerationCallEvent::IN_PROGRESS,
             searching_event: Some(ImageGenerationCallEvent::GENERATING),
             completed_event: ImageGenerationCallEvent::COMPLETED,
@@ -64,7 +84,7 @@ pub const fn descriptor(format: ResponseFormat) -> FormatDescriptor {
         },
         ResponseFormat::Passthrough => FormatDescriptor {
             type_str: ItemType::MCP_CALL,
-            id_prefix: "mcp_",
+            id_prefix: "mcp",
             in_progress_event: McpEvent::CALL_IN_PROGRESS,
             searching_event: None,
             completed_event: McpEvent::CALL_COMPLETED,

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

@@ -5,6 +5,7 @@ use std::sync::Arc;
 
 use dashmap::DashMap;
 use smg_mcp::{inventory::ALIAS_SERVER_KEY, McpServerConfig, QualifiedToolName};
+use tracing::debug;
 
 use super::ResponseFormat;
 
@@ -16,6 +17,12 @@ use super::ResponseFormat;
 /// `QualifiedToolName` returned by `qualified_name_for_exposed` rather than
 /// rebuilding one, so we pay the two `Arc<str>` allocations once instead of
 /// twice per call.
+///
+/// Telemetry: when the session knows the tool but the registry doesn't, we
+/// log a `debug` event with the qualified name. That asymmetric miss is the
+/// fingerprint of a registration-path bug (the orchestrator added the tool
+/// but `populate_from_server_config` was never called for the same server),
+/// and would otherwise dispatch silently as `mcp_call`.
 pub fn lookup_tool_format(
     session: &smg_mcp::McpToolSession<'_>,
     registry: &FormatRegistry,
@@ -24,7 +31,17 @@ pub fn lookup_tool_format(
     let Some(qn) = session.qualified_name_for_exposed(exposed_name) else {
         return ResponseFormat::Passthrough;
     };
-    registry.lookup(&qn)
+    let format = registry.lookup(&qn);
+    if format == ResponseFormat::Passthrough && !registry.contains(&qn) {
+        debug!(
+            exposed_name = %exposed_name,
+            server_key = %qn.server_key(),
+            tool_name = %qn.tool_name(),
+            "FormatRegistry miss for session-exposed tool — dispatching as Passthrough; \
+             check that populate_from_server_config ran for this server"
+        );
+    }
+    format
 }
 
 #[derive(Default, Debug, Clone)]
@@ -48,6 +65,13 @@ impl FormatRegistry {
         self.lookup(&QualifiedToolName::new(server_key, tool_name))
     }
 
+    /// True iff the registry has an explicit entry for `qualified`. Used by
+    /// [`lookup_tool_format`] to distinguish a registered Passthrough entry
+    /// from a missing entry that defaulted to Passthrough.
+    pub fn contains(&self, qualified: &QualifiedToolName) -> bool {
+        self.formats.contains_key(qualified)
+    }
+
     fn insert(&self, qualified: QualifiedToolName, format: ResponseFormat) {
         self.formats.insert(qualified, format);
     }
@@ -111,8 +135,10 @@ impl FormatRegistry {
 mod tests {
     use std::collections::HashMap;
 
+    use serde_json::json;
     use smg_mcp::{
-        BuiltinToolType, McpServerConfig, McpTransport, ResponseFormatConfig, ToolConfig,
+        BuiltinToolType, McpConfig, McpOrchestrator, McpServerBinding, McpServerConfig,
+        McpToolSession, McpTransport, ResponseFormatConfig, Tool, ToolConfig, ToolEntry,
     };
 
     use super::*;
@@ -134,6 +160,21 @@ mod tests {
         }
     }
 
+    fn test_tool(name: &str) -> Tool {
+        let mut schema = serde_json::Map::new();
+        schema.insert("type".to_string(), json!("object"));
+        schema.insert("properties".to_string(), json!({}));
+        Tool {
+            name: name.to_string().into(),
+            title: None,
+            description: Some("test".into()),
+            input_schema: schema.into(),
+            output_schema: None,
+            icons: None,
+            annotations: None,
+        }
+    }
+
     #[test]
     fn lookup_unknown_returns_passthrough() {
         let r = FormatRegistry::new();
@@ -311,4 +352,82 @@ mod tests {
             ResponseFormat::Passthrough,
         );
     }
+
+    async fn orchestrator_with_tool(server_name: &str, tool_name: &str) -> McpOrchestrator {
+        let orchestrator = McpOrchestrator::new(McpConfig {
+            servers: vec![server(server_name)],
+            ..Default::default()
+        })
+        .await
+        .expect("orchestrator");
+        orchestrator
+            .tool_inventory()
+            .insert_entry(ToolEntry::from_server_tool(
+                server_name,
+                test_tool(tool_name),
+            ));
+        orchestrator
+    }
+
+    fn binding(server_name: &str) -> Vec<McpServerBinding> {
+        vec![McpServerBinding {
+            label: server_name.to_string(),
+            server_key: server_name.to_string(),
+            allowed_tools: None,
+        }]
+    }
+
+    #[tokio::test]
+    async fn lookup_tool_format_returns_passthrough_when_session_unknown() {
+        let orchestrator = orchestrator_with_tool("brave", "brave_web_search").await;
+        let session = McpToolSession::new(&orchestrator, binding("brave"), "test-request");
+        let registry = FormatRegistry::new();
+        // `not_a_tool` was never registered with the session, so the lookup
+        // short-circuits at `qualified_name_for_exposed`.
+        assert_eq!(
+            lookup_tool_format(&session, &registry, "not_a_tool"),
+            ResponseFormat::Passthrough
+        );
+    }
+
+    #[tokio::test]
+    async fn lookup_tool_format_returns_passthrough_for_session_known_but_registry_missing() {
+        // Asymmetric-miss branch: the session exposes the tool but the
+        // registry has no entry — production fingerprint of a server whose
+        // `populate_from_server_config` was skipped. Must dispatch as
+        // Passthrough rather than panicking or making up a hosted format.
+        let orchestrator = orchestrator_with_tool("brave", "brave_web_search").await;
+        let session = McpToolSession::new(&orchestrator, binding("brave"), "test-request");
+        let registry = FormatRegistry::new();
+        assert_eq!(
+            lookup_tool_format(&session, &registry, "brave_web_search"),
+            ResponseFormat::Passthrough
+        );
+    }
+
+    #[tokio::test]
+    async fn lookup_tool_format_returns_registry_value_for_session_known_and_registered() {
+        // Happy path: session knows the tool AND registry has a hosted entry —
+        // composed lookup returns the hosted format.
+        let orchestrator = orchestrator_with_tool("brave", "brave_web_search").await;
+        let session = McpToolSession::new(&orchestrator, binding("brave"), "test-request");
+        let mut tools = HashMap::new();
+        tools.insert(
+            "brave_web_search".to_string(),
+            ToolConfig {
+                alias: None,
+                response_format: Some(ResponseFormatConfig::WebSearchCall),
+                arg_mapping: None,
+            },
+        );
+        let mut cfg = server("brave");
+        cfg.tools = Some(tools);
+        let registry = FormatRegistry::new();
+        registry.populate_from_server_config(&cfg);
+
+        assert_eq!(
+            lookup_tool_format(&session, &registry, "brave_web_search"),
+            ResponseFormat::WebSearchCall
+        );
+    }
 }

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

@@ -10,14 +10,15 @@ pub mod response_format;
 pub mod tool_descriptors;
 pub mod transformer;
 
-pub use format_descriptor::{descriptor, FormatDescriptor};
+pub use format_descriptor::{descriptor, format_from_type_str, FormatDescriptor};
 pub use format_registry::{lookup_tool_format, FormatRegistry};
 pub use overrides::{apply_hosted_tool_overrides, extract_hosted_tool_overrides};
 pub use response_format::ResponseFormat;
 pub use tool_descriptors::{
-    build_mcp_tool_infos, chat_function_tools, configure_response_tools_approval,
-    function_tools_json, inject_client_visible_mcp_output_items, mcp_list_tools_item,
-    mcp_list_tools_json, response_tools, should_hide_output_item_json, should_hide_tool_json,
+    build_mcp_tool_infos, builtin_type_for_response_tool, chat_function_tools,
+    configure_response_tools_approval, function_tools_json, inject_client_visible_mcp_output_items,
+    mcp_list_tools_item, mcp_list_tools_json, response_tools, should_hide_output_item_json,
+    should_hide_tool_json,
 };
 pub use transformer::{
     compact_image_generation_output, compact_image_generation_outputs_json,

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

@@ -14,7 +14,24 @@ use openai_protocol::{
     },
 };
 use serde_json::{json, Value};
-use smg_mcp::{ApprovalMode, McpToolSession};
+use smg_mcp::{ApprovalMode, BuiltinToolType, McpToolSession};
+
+/// Map a single `ResponseTool` variant to the `BuiltinToolType` the MCP
+/// router uses for resolution. `None` for non-builtin tool kinds (e.g.
+/// `Function`, `Mcp`, `Computer`). Centralizing here keeps the four
+/// hosted families (web_search_preview / code_interpreter / file_search /
+/// image_generation) in lockstep — every caller that previously matched
+/// on `ResponseTool::*` independently was at risk of forgetting a variant
+/// when a new builtin lands.
+pub fn builtin_type_for_response_tool(tool: &ResponseTool) -> Option<BuiltinToolType> {
+    match tool {
+        ResponseTool::WebSearchPreview(_) => Some(BuiltinToolType::WebSearchPreview),
+        ResponseTool::CodeInterpreter(_) => Some(BuiltinToolType::CodeInterpreter),
+        ResponseTool::FileSearch(_) => Some(BuiltinToolType::FileSearch),
+        ResponseTool::ImageGeneration(_) => Some(BuiltinToolType::ImageGeneration),
+        _ => None,
+    }
+}
 
 #[inline]
 fn schema_to_value(schema: &serde_json::Map<String, Value>) -> Value {