refactor(openai-bridge): close last new-builtin coupling leaks (descriptor + connect wrapper)

[slin1237]

Summary

• Closes the last two ways adding a built-in tool can drift outside openai_bridge/:
  • tool_handler.rs no longer hand-maintains TOOL_CALL_ITEM_TYPES. The suppression gate now sources its hosted set from openai_bridge::is_hosted_tool_call_item_type, which derives from the descriptor table.
  • The McpOrchestrator::connect_* + FormatRegistry::populate_from_server_config pair is collapsed into one wrapper (openai_bridge::connect_static_server / connect_dynamic_server) so a future connect path can't silently leave the registry behind.

Motivation

After PR #1450 merged, two residual leaks were left from the mcp-refactor design:

1. tool_handler.rs:23 TOOL_CALL_ITEM_TYPES — a hand-maintained list of every item type whose upstream output_item.done umbrella the streaming tool loop suppresses. Adding a new hosted family without remembering to update this const lets the upstream's duplicate umbrella slip through ahead of <type>.completed, breaking the spec's "umbrella is the LAST event" invariant.
2. populate_from_server_config parity — connect paths in mcp_utils.rs and workflow/mcp_registration.rs each had to remember to call format_registry.populate_from_server_config after orchestrator.connect_*. Forgetting the second call silently downgrades hosted-tool dispatch to mcp_call. The 283f27f debug log catches it at runtime, but a wrapper makes the bug structurally impossible.

Both were the kind of thing reviewers would not catch — neither is a compile error.

Changes

• openai_bridge/format_descriptor.rs — add is_hosted_tool_call_item_type derived from format_from_type_str.
• openai_bridge/connect.rs — new module with connect_static_server + connect_dynamic_server wrappers.
• openai_bridge/mod.rs — re-exports.
• openai/mcp/tool_handler.rs — replace const + helper with the descriptor-derived check; add a regression test pinning the 6 hits + 5 misses.
• routers/common/mcp_utils.rs — migrate dynamic-connect site to the wrapper.
• workflow/mcp_registration.rs — migrate static-connect site to the wrapper.

Test plan

• cargo test -p smg --lib — 751 passed
• cargo clippy -p smg --lib --tests -- -D warnings — clean
• cargo fmt -p smg — clean
• New regression test is_tool_call_item_type_covers_function_call_and_hosted_builtins locks the 6-item suppression set against silent drift

Summary by CodeRabbit

• Refactor
  • Bridge-based MCP server connections now centralize registration updates for more consistent connectivity behavior.
  • Tool-call detection broadened to recognize hosted built-in types alongside function-call types for more accurate request handling.
  • Internal module organization and re-exports clarified to improve maintainability and surface the new bridge helpers.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 406d88fd-4276-4a00-b012-6394f64fd021

📥 Commits

Reviewing files that changed from the base of the PR and between 3a611c0 and 98b6f41.

📒 Files selected for processing (5)

• model_gateway/src/routers/common/mcp_utils.rs
• model_gateway/src/routers/common/openai_bridge/connect.rs
• model_gateway/src/routers/common/openai_bridge/format_descriptor.rs
• model_gateway/src/routers/openai/mcp/tool_handler.rs
• model_gateway/src/workflow/mcp_registration.rs

---

📝 Walkthrough

Walkthrough

This PR moves FormatRegistry synchronization into new openai_bridge connect helpers and replaces a static tool-call item-type list with a dynamic check that recognizes hosted tool-call formats alongside function_call types; tests and module re-exports updated accordingly.

Changes

Bridge + Tool-Call Integration

Layer / File(s)	Summary
Bridge Connection Helpers model_gateway/src/routers/common/openai_bridge/connect.rs	Adds connect_static_server and connect_dynamic_server wrappers that call the orchestrator and then registry.populate_from_server_config to keep registry in sync.
Module Exports model_gateway/src/routers/common/openai_bridge/mod.rs	Adds pub mod connect and re-exports connect_dynamic_server, connect_static_server; re-exports is_hosted_tool_call_item_type.
Format Descriptor model_gateway/src/routers/common/openai_bridge/format_descriptor.rs	Adds pub fn is_hosted_tool_call_item_type(item_type: &str) -> bool using format_from_type_str → to_builtin_tool_type to detect hosted tool-call types.
MCP Utils Integration model_gateway/src/routers/common/mcp_utils.rs	Dynamic MCP connection now delegates to openai_bridge::connect_dynamic_server(...), moving server_config ownership into the bridge and removing local populate_from_server_config call.
Registration Integration model_gateway/src/workflow/mcp_registration.rs	Static MCP connect now uses openai_bridge::connect_static_server(... ) and no longer calls populate_from_server_config directly.
Tool Handler Logic & Tests model_gateway/src/routers/openai/mcp/tool_handler.rs	Replaces hard-coded TOOL_CALL_ITEM_TYPES check with `is_function_call_type(...)

Sequence Diagram(s)

sequenceDiagram
    participant Router as OpenAI Router
    participant Bridge as openai_bridge
    participant Orch as McpOrchestrator
    participant Reg as FormatRegistry

    Router->>Bridge: connect_dynamic_server(config)
    Bridge->>Orch: orchestrator.connect_dynamic_server(config.clone())
    Orch-->>Bridge: server_key
    Bridge->>Reg: populate_from_server_config(&config)
    Bridge-->>Router: server_key

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• lightseekorg/smg#1450: Related changes to openai_bridge descriptor APIs and registry wiring relied upon by these connect helpers.
• lightseekorg/smg#1370: Hosted-tool classification and ResponseFormat → builtin tool-type wiring that is_hosted_tool_call_item_type depends on.
• lightseekorg/smg#1371: Overlaps on tool-call item-type detection changes in tool_handler.rs.

Suggested labels

mcp, tests

Suggested reviewers

• CatherineSue
• key4ng

Poem

🐰 The bridge now hums beneath the code,
Registry and server walk the road,
Tool-calls found both hosted and named,
One connect call keeps data framed,
A tiny hop — the gateway glowed.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: refactoring the openai-bridge to eliminate coupling issues by introducing a descriptor-based predicate and wrapper functions for server connections.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/openai-bridge-tool-call-types-from-descriptor

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Clean refactor — wrapping connect + registry-populate into bridge helpers eliminates a class of silent-downgrade bugs, and deriving the tool-call suppression set from the descriptor table keeps the single source of truth. All gateway-layer call sites migrated, test coverage pins the expected set. LGTM.

[gemini-code-assist]

Code Review

This pull request introduces atomic helper functions in a new openai_bridge::connect module to ensure that the McpOrchestrator and FormatRegistry remain synchronized during MCP server connections. It also refactors tool-call item type checking to use centralized logic in openai_bridge, replacing hardcoded lists in the tool handler. Feedback suggests optimizing connect_dynamic_server by reordering operations to avoid an unnecessary clone of the server configuration.

[gemini-code-assist]

To avoid an unnecessary clone of McpServerConfig, which can be a relatively large struct containing multiple HashMaps and Strings, you can populate the FormatRegistry before moving the configuration into the orchestrator. Since FormatRegistry::populate_from_server_config is synchronous and lookup_tool_format (the primary consumer) verifies tool existence against the session inventory, populating the registry slightly earlier is safe even if the subsequent connection attempt fails.

pub async fn connect_dynamic_server(
    orchestrator: &McpOrchestrator,
    registry: &FormatRegistry,
    config: McpServerConfig,
) -> McpResult<String> {
    registry.populate_from_server_config(&config);
    orchestrator.connect_dynamic_server(config).await
}