Skip to content

Conversation

@HRNPH
Copy link

@HRNPH HRNPH commented Dec 25, 2025

  • Added MCP Tool calling support
  • Backward compatible with previous tools system as it is separate arguments

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds Model Context Protocol (MCP) tool integration to the OpenAI Realtime provider, enabling the SDK to connect to remote MCP servers and expose their tools to OpenAI's realtime API. The implementation maintains backward compatibility by keeping MCP servers as a separate configuration option alongside the existing manual tools system.

Key changes:

  • New MCPToolManager class handles connections to multiple MCP servers, tool discovery with pagination support, and tool execution routing
  • OpenAIRealtimeProvider now manages both manual and MCP tools separately, merging them during session configuration
  • Configuration extended with MCPServerConfig interface supporting streamable HTTP transport, authentication headers, and configurable tool name prefixing

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
packages/providers/openai-realtime/tsconfig.json Added module resolution overrides and path mapping for @khaveeai/core to support MCP SDK imports
packages/providers/openai-realtime/src/mcp/MCPToolManager.ts New class managing MCP server connections, tool discovery, schema conversion, and execution routing
packages/providers/openai-realtime/src/OpenAIRealtimeProvider.ts Integrated MCP tools alongside manual tools with separate tracking, async initialization, and merged registration
packages/providers/openai-realtime/package.json Added @modelcontextprotocol/sdk dependency
packages/providers/openai-realtime/README.md Added documentation for MCP server configuration and usage examples
packages/core/src/types/realtime.ts Added MCPServerConfig interface and mcpServers optional field to RealtimeConfig
README.md Added high-level MCP tools documentation with configuration examples

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +283 to +285
[...this.manualTools, ...this.mcpTools].forEach((tool) => {
combined.set(tool.name, tool);
});
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When merging manual and MCP tools in getAllRegisteredTools, if both sources provide a tool with the same name, the later one (MCP tools) silently overwrites the earlier one (manual tools) due to Map behavior. This could lead to unexpected behavior if users define a manual tool with the same name as an MCP tool. Consider warning about or preventing name collisions, or documenting the precedence order.

Suggested change
[...this.manualTools, ...this.mcpTools].forEach((tool) => {
combined.set(tool.name, tool);
});
// First, register all manual tools.
this.manualTools.forEach((tool) => {
combined.set(tool.name, tool);
});
// Then, register MCP tools, warning on name collisions and
// preserving the existing behavior where MCP tools take precedence.
this.mcpTools.forEach((tool) => {
if (combined.has(tool.name)) {
// Name collision: MCP tool overrides a manual tool with the same name.
console.warn(
`[OpenAIRealtimeProvider] Tool name collision detected for "${tool.name}". ` +
"The MCP tool will override the previously registered manual tool."
);
}
combined.set(tool.name, tool);
});

Copilot uses AI. Check for mistakes.
const requiredFields: string[] = [];

Object.entries(tool.parameters).forEach(
Object.entries(tool.parameters || {}).forEach(
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The safeguard 'tool.parameters || {}' is good, but this change suggests that tool.parameters might be undefined. However, the RealtimeTool type definition requires parameters to always be present (not optional). If parameters can indeed be undefined in practice, the type definition should be updated to reflect this, otherwise this null check is unnecessary.

Suggested change
Object.entries(tool.parameters || {}).forEach(
Object.entries(tool.parameters).forEach(

Copilot uses AI. Check for mistakes.
Comment on lines +6 to +10
"module": "nodenext",
"moduleResolution": "nodenext",
"baseUrl": ".",
"paths": {
"@khaveeai/core": ["../../core/dist/index.d.ts"]
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tsconfig overrides module and moduleResolution to "nodenext", which conflicts with the parent config's "esnext" and "node" settings. This change affects how imports are resolved and may cause compatibility issues. The paths configuration pointing to a specific .d.ts file is also unusual - typically paths point to directories, not individual declaration files. Consider whether these overrides are necessary or if they could cause build inconsistencies with other packages in the monorepo.

Suggested change
"module": "nodenext",
"moduleResolution": "nodenext",
"baseUrl": ".",
"paths": {
"@khaveeai/core": ["../../core/dist/index.d.ts"]
"baseUrl": ".",
"paths": {
"@khaveeai/core": ["../../core/dist"]

Copilot uses AI. Check for mistakes.
Comment on lines 265 to +267
registerFunction(tool: RealtimeTool): void {
this.registerTool(tool, "manual");
}
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a potential race condition if registerFunction is called externally after connect but before MCP tools are initialized. The configureSession method waits for MCP initialization, but if someone calls registerFunction and it triggers a reconfiguration, the new manual tool won't be sent to OpenAI until the next session update. Consider documenting that tools should be registered before calling connect, or ensure session updates when tools are registered after initial configuration.

Copilot uses AI. Check for mistakes.
- `id`: unique identifier. Also used as the default prefix for tools (`${id}__toolName`).
- `url`: Streamable HTTP endpoint for the server (e.g., `https://.../mcp`).
- `headers`: optional HTTP headers for auth.
- `toolNamePrefix`: override the default prefix (set to `null` to keep original names).
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When toolNamePrefix is set to null to preserve original tool names, there's a risk of name collisions between tools from different MCP servers or between MCP tools and manual tools. The code handles this by overwriting (last one wins in the Map), but users aren't warned about this in the documentation. Consider adding a note about the collision risk when using null prefix.

Suggested change
- `toolNamePrefix`: override the default prefix (set to `null` to keep original names).
- `toolNamePrefix`: override the default prefix (set to `null` to keep original names). **Warning:** when set to `null`, tool names are not namespaced, so tools from different MCP servers or manual tools can collide; in case of a collision, the last registered tool overwrites earlier ones.

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +70
} catch {
// Ignore shutdown errors
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handler silently swallows shutdown errors without any logging. While it's reasonable to ignore errors during cleanup, completely silent failures make debugging connection issues difficult. Consider adding a debug log or at least incrementing an error counter to track cleanup failures.

Suggested change
} catch {
// Ignore shutdown errors
} catch (error) {
console.debug(
`[MCP] Error while closing transport for server '${entry.server.id}':`,
error
);

Copilot uses AI. Check for mistakes.
version: server.client?.version || "1.0.0",
},
{
capabilities: (server.client?.capabilities as any) || {},
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type assertion 'as any' bypasses TypeScript's type checking for the capabilities object. While the MCP SDK may accept a flexible capabilities structure, this could hide type mismatches or invalid configurations. Consider defining a proper type for capabilities or at least using 'as unknown' to make the unsafe cast more explicit.

Suggested change
capabilities: (server.client?.capabilities as any) || {},
capabilities: (server.client?.capabilities as unknown) || {},

Copilot uses AI. Check for mistakes.
Comment on lines +237 to +239
return {
success: !isError,
message: message || (isError ? "MCP tool call failed." : "MCP tool executed successfully."),
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method returns a hardcoded success message when isError is false and no message content is present. This could be misleading if the tool execution actually returns no output. Consider differentiating between an empty successful result and a tool that provides no feedback, or include the tool name in the message for better traceability.

Suggested change
return {
success: !isError,
message: message || (isError ? "MCP tool call failed." : "MCP tool executed successfully."),
let finalMessage = message;
if (!finalMessage) {
if (isError) {
finalMessage = `MCP tool '${toolName}' on '${serverId}' failed without providing an error message.`;
} else {
finalMessage = `MCP tool '${toolName}' on '${serverId}' completed successfully but did not return any output.`;
}
}
return {
success: !isError,
message: finalMessage,

Copilot uses AI. Check for mistakes.
Comment on lines +76 to 86
if (this.config.mcpServers && this.config.mcpServers.length > 0) {
this.mcpManager = new MCPToolManager(this.config.mcpServers);
this.mcpInitialization = this.mcpManager
.initialize()
.then((tools) => {
tools.forEach((tool) => this.registerTool(tool, "mcp"));
})
.catch((error) => {
console.error("Failed to initialize MCP servers:", error);
});
}
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MCP initialization is started in the constructor but not awaited, creating a fire-and-forget promise. If initialization fails quickly (e.g., invalid URL format), the error is logged but there's no way for calling code to know initialization has failed until they try to use the tools. Consider exposing the initialization promise or adding a ready state flag so consumers can check if MCP is fully initialized.

Copilot uses AI. Check for mistakes.
Comment on lines +613 to +619
apiKey: process.env.NEXT_PUBLIC_OPENAI_API_KEY!,
mcpServers: [
{
id: "docs",
url: process.env.NEXT_PUBLIC_DOCS_MCP_URL!,
headers: {
Authorization: `Bearer ${process.env.NEXT_PUBLIC_DOCS_MCP_TOKEN!}`,
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example encourages putting NEXT_PUBLIC_OPENAI_API_KEY and NEXT_PUBLIC_DOCS_MCP_TOKEN into NEXT_PUBLIC_* environment variables, which are bundled into client-side JavaScript and fully exposed to any user of the app. An attacker can trivially extract these values from the browser, then reuse your OpenAI API key and MCP token to call your backends, exhaust quotas, or access data. To avoid leaking these secrets, keep them in server-only configuration (non-public env vars or a backend proxy) and do not expose them to untrusted clients.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants