-
-
Notifications
You must be signed in to change notification settings - Fork 41
[Feature] MCP client reconnection and Gemini API improvements #635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…dling - Fix TypeScript type errors in filterKeys usage for function calling parts - Add proper typing using ChatFunctionCallingPart and ChatFunctionResponsePart - Conditionally include tool call IDs based on useCamelCaseSystemInstruction setting - Add generic type parameters to ChatLunaPlugin and ModelRequester for better type safety - Update locale descriptions for image generation and Google search features - Import filterKeys and notNullFn locally instead of from koishi package
…ling - Implement automatic reconnection with exponential backoff for MCP clients - Add ToolListChangedNotification handler for dynamic tool updates - Add proper connection close event handling - Track reconnection attempts with configurable max retries - Support incremental tool registration and replacement - Improve type safety in tool output processing - Upgrade @modelcontextprotocol/sdk to v1.23.0 - Add Claude Opus 4.5 model support in adapter-claude - Fix GeminiClient plugin type annotation This update improves the robustness of MCP client connections by handling disconnections gracefully and supporting dynamic tool list changes without requiring full restarts.
|
Warning Rate limit exceeded@dingyi222666 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough新增 Claude 模型条目;Gemini 相关类型签名被泛型化并精化函数调用处理以支持可选去除函数调用 id 和 camelCase 系统指令;核心请求器泛型化以匹配适配器改动;MCP 扩展重构为按服务器的连接生命周期、重连与增量工具注册并改进工具输出访问。 Changes
Sequence Diagram(s)sequenceDiagram
participant User as 用户
participant MCPService as MCP Service
participant ServerConfig as 服务器配置
participant Transport as 传输层
participant MCPClient as MCP Client
participant ToolRegistry as 工具注册表
participant Reconnect as 重连机制
User->>MCPService: 启动服务
MCPService->>ServerConfig: 解析/读取每服务器配置
ServerConfig-->>MCPService: 返回配置
rect rgb(200,220,255)
Note over MCPService,Transport: 为每服务器创建传输并连接
MCPService->>Transport: _createTransport(config)
Transport-->>MCPService: 传输实例
MCPService->>MCPClient: 创建并 connect()
MCPClient-->>MCPService: 连接建立 / 事件订阅
end
rect rgb(220,240,200)
Note over MCPService,ToolRegistry: 初始与增量工具注册
MCPService->>MCPClient: listTools()
MCPClient-->>MCPService: 返回工具列表
MCPService->>ToolRegistry: _registerToolsForClient(工具列表)
ToolRegistry-->>MCPService: 注册完成(新增/替换)
end
MCPClient->>MCPService: ToolListChangedNotification
MCPService->>MCPService: _handleToolListChange -> listTools()
alt 工具列表为空
MCPService->>Reconnect: _scheduleReconnect(指数退避)
else 有变更
MCPService->>ToolRegistry: 增量更新注册
end
MCPClient-->>MCPService: 连接错误/断开
MCPService->>Reconnect: 启动重连尝试(指数退避)
alt 重连成功
Reconnect-->>MCPService: 连接恢复
MCPService->>ToolRegistry: 重新注册/同步工具
else 超过最大重试
Reconnect-->>MCPService: 放弃并记录
end
User->>MCPService: 停止服务
MCPService->>Reconnect: 清除重连计时器
MCPService->>MCPClient: 关闭所有客户端连接
MCPClient-->>MCPService: 断开确认
MCPService->>ToolRegistry: 重置内部状态
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 分钟 需要重点检查:
Possibly related PRs
诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @dingyi222666, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the reliability and functionality of the ModelContext Protocol (MCP) client integration and refining the Gemini adapter. It introduces robust reconnection mechanisms for MCP clients, enabling them to gracefully recover from connection losses and dynamically adapt to changes in available tools. Concurrently, the Gemini adapter receives critical type safety improvements and better handling of function calls, ensuring more stable and predictable interactions with Gemini models. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Version bumps for recent feature additions and bug fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant improvements, including robust MCP client reconnection capabilities with exponential backoff and dynamic tool list handling. The Gemini adapter has also been enhanced with better type safety and fixes for function calling. The refactoring in packages/extension-mcp/src/service.ts is particularly well-done, making the code more modular and maintainable. I've identified one critical logic bug in packages/extension-mcp/src/utils.ts that needs to be addressed, as it could lead to runtime errors. I also found a minor typo in a new Claude model name. Overall, these are valuable additions and fixes.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/extension-mcp/src/service.ts (3)
184-186:client.close()触发onclose会导致重复调度重连。调用
await client.close()会触发onclose处理器,该处理器已经执行了_clients.delete()和_scheduleReconnect()。这里再次调用这两个方法是冗余的。虽然_scheduleReconnect会清除已有的定时器所以功能上正常,但逻辑上会造成混淆。建议移除冗余调用,让
onclose处理器统一处理重连逻辑:logger.warn( `Tool list is empty for server: ${JSON.stringify(serverConfig)}, triggering full reconnect...` ) await client.close() - this._clients.delete(serverConfig) - this._scheduleReconnect(serverConfig) + // onclose handler will handle cleanup and reconnection return }
244-250: URL 中包含 'sse' 的启发式检测可能误判。
url.includes('sse')可能错误匹配不相关的 URL(如https://messages-server.com)。建议优先使用type字段判断:if (url == null) { return this._createStdioTransport(command, args, env, cwd) - } else if (url.includes('sse') || type?.includes('sse')) { + } else if (type?.includes('sse') || (!type && url.includes('sse'))) { return this._createSSETransport(url, headers, proxy) } else if (url.startsWith('http')) { return this._createHTTPTransport(url, headers, proxy) }
276-284: 对象类型的空值检测可能不符合预期。
parsedArgs[key].toString()对对象会返回'[object Object]',所以空对象{}不会被过滤掉。如果需要过滤空对象,可以增加额外检查。当前实现对StdioClientTransport来说应该不会造成问题。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/adapter-gemini/src/locales/en-US.schema.ymlis excluded by!**/*.ymlpackages/adapter-gemini/src/locales/zh-CN.schema.ymlis excluded by!**/*.ymlpackages/extension-mcp/package.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
packages/adapter-claude/src/client.ts(1 hunks)packages/adapter-gemini/src/client.ts(1 hunks)packages/adapter-gemini/src/requester.ts(1 hunks)packages/adapter-gemini/src/utils.ts(8 hunks)packages/core/src/llm-core/platform/api.ts(1 hunks)packages/extension-mcp/src/service.ts(6 hunks)packages/extension-mcp/src/utils.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T17:40:57.183Z
Learnt from: dingyi222666
Repo: ChatLunaLab/chatluna PR: 546
File: packages/core/src/llm-core/chat/app.ts:114-119
Timestamp: 2025-09-15T17:40:57.183Z
Learning: 在 ChatLuna 的 packages/core/src/llm-core/chat/app.ts 中,displayResponse 的构造只需要保留 content 和 additional_kwargs,tool_calls、tool_call_id、name、id 等元数据在代码库的其他地方处理,无需在 displayResponse 中保留这些字段。
Applied to files:
packages/adapter-gemini/src/utils.ts
🧬 Code graph analysis (4)
packages/core/src/llm-core/platform/api.ts (1)
packages/core/src/services/chat.ts (1)
ChatLunaPlugin(594-800)
packages/extension-mcp/src/service.ts (2)
packages/extension-mcp/src/index.ts (3)
Config(21-39)Config(41-66)logger(8-8)packages/extension-mcp/src/utils.ts (1)
callTool(438-501)
packages/adapter-gemini/src/client.ts (3)
packages/core/src/services/chat.ts (1)
ChatLunaPlugin(594-800)packages/core/src/llm-core/platform/config.ts (1)
ClientConfig(9-17)packages/adapter-gemini/src/index.ts (2)
Config(39-54)Config(56-95)
packages/adapter-gemini/src/utils.ts (3)
packages/core/src/services/chat.ts (1)
ChatLunaPlugin(594-800)packages/core/src/llm-core/platform/config.ts (1)
ClientConfig(9-17)packages/adapter-gemini/src/types.ts (2)
ChatFunctionCallingPart(46-53)ChatFunctionResponsePart(55-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (18)
packages/extension-mcp/src/service.ts (4)
36-46: LGTM! 重连状态管理的字段设计合理,使用 Map 跟踪每个服务器的重连状态是正确的做法。
141-171: 事件处理器设置正确。
onclose处理器能正确检测_isStopped状态来区分主动关闭和意外断开。ToolListChangedNotification订阅实现了动态工具列表更新。
203-238: 指数退避重连实现正确。退避策略
Math.min(1000 * Math.pow(2, attempts), 30000)合理地控制了重试间隔(1s → 2s → 4s → 8s → 16s → 30s 封顶)。重连成功后重新注册工具的逻辑也正确。
445-461: 停止逻辑实现完善。正确地先设置
_isStopped = true再关闭客户端,确保onclose处理器不会触发重连。定时器和状态的清理顺序合理。packages/adapter-gemini/src/utils.ts (6)
15-16: LGTM!新增的类型导入支持了重构后的函数调用处理逻辑,类型定义清晰。
34-34: LGTM!导入 ClientConfig 类型以支持泛型类型参数,提升了类型安全性。
38-38: LGTM!泛型类型参数的添加提升了类型安全性,与 ModelRequester 基类的泛型化保持一致。
Also applies to: 498-498
129-176: LGTM!函数重构逻辑正确:先构建
functionCall和functionResponse对象,然后根据removeId参数条件性地添加id字段。类型注解与导入的类型定义一致。不过请注意,此函数签名的变更与第 49-52 行的语义混淆问题相关联。
490-490: LGTM!使用
notNullFn辅助函数替代内联过滤,代码更简洁、更具声明性。
543-555: LGTM!辅助函数实现正确:
notNullFn提供了类型安全的非空检查filterKeys实现了通用的对象键值过滤功能两个函数的类型签名和实现都符合最佳实践。
packages/adapter-gemini/src/client.ts (1)
35-35: LGTM!为
plugin参数添加泛型类型ChatLunaPlugin<ClientConfig, Config>,与整体的类型安全改进保持一致。packages/core/src/llm-core/platform/api.ts (1)
99-99: LGTM!基类
ModelRequester的_plugin参数现在使用泛型类型ChatLunaPlugin<T, R>,这是一个重要的类型安全改进,使得子类能够正确指定其特定的配置类型。packages/adapter-gemini/src/requester.ts (1)
51-51: LGTM!
GeminiRequester现在正确地使用泛型参数扩展ModelRequester<ClientConfig, Config>,构造函数参数类型也相应更新。这些更改与基类的泛型化改进保持一致。Also applies to: 58-58
packages/extension-mcp/src/utils.ts (5)
79-96: LGTM - 类型安全改进将属性访问从点号改为方括号表示法(
resource['blob']和resource['text'])是为了处理联合类型的类型安全问题。当资源类型是联合类型且某些成员不包含 blob/text 属性时,方括号访问可以避免 TypeScript 编译错误。运行时行为保持不变,这是一个良好的类型安全改进。
101-134: LGTM - 增强类型安全性将函数参数类型从通用类型改为
CallToolResult['content'][0]是一个很好的类型安全改进,确保参数类型与 MCP SDK 的实际返回类型严格匹配。这种显式类型约束有助于在编译时捕获类型不匹配问题。
236-240: LGTM - 保持一致性此处改用方括号访问
content['type']与文件中其他类型安全改进保持一致,有助于处理联合类型时的类型检查。
342-346: LGTM - 类型安全改进将
content.text改为content['text']与文件中的其他更改保持一致,提高了处理联合类型时的类型安全性。
53-55: Now I have all the information I need. The review comment shows a snippet with OR operators, but the actual current code in the repository uses AND operators and is logically correct.
The code snippet in this review does not match the current state of the repository.
The actual current code at lines 53-55 uses AND operators (
&&), not OR (||) as shown in the review snippet:resource.uri != null && resource['blob'] == null && resource['text'] == nullThis logic is correct. According to the MCP SDK specification,
uriis always required on ResourceContents, andtextandblobare mutually exclusive. The function properly identifies uri-only references (without embedded content) by checking that uri exists while both blob and text are null. When true, the code fetches the resource viaclient.readResource(); when false, it processes the embedded blob or text directly.Likely an incorrect or invalid review comment.
- Fix typo in Claude Opus 4.5 model ID (2025110 -> 20251101) - Bump adapter-claude version to 1.3.1 - Bump extension-mcp version to 1.3.2
Add clarification comment about removing ID when using new API for function messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/extension-mcp/src/utils.ts (3)
79-96: 嵌入资源到 StandardFileBlock 的改动仅为类型兼容,逻辑OK
resource['blob']/resource['text']的索引访问方式有助于绕过 TS 对联合类型属性的限制,生成的StandardFileBlock & Base64ContentBlock/PlainTextContentBlock结构与之前保持一致。建议后续如果有机会,可以通过为资源类型补充更精确的定义来避免string索引访问,但不影响当前功能。
235-240: 错误消息中使用content['type']访问类型字段是合理的折中这里改为索引访问
content['type']主要是为了解决 TS 对联合类型type字段的推断问题,生成的日志格式与原先保持一致。注意如果后续 MCP SDK 新增了其他内容块类型,建议同步更新错误提示中的“Expected one of ...”列表以保持准确。
341-346: 错误结果聚合时直接取content['text']可能得到undefined当前在
result.isError分支里对所有result.content执行.map((content) => content['text']),如果某些错误内容块不是纯文本(或者未来 SDK 扩展了错误内容类型),拼接出来的消息中可能包含'undefined'行,降低可读性。建议至少先过滤type === 'text'的块,或在缺少text时退回JSON.stringify(content):- ${result.content - .map((content: CallToolResult['content'][0]) => content['text']) - .join('\n')} + ${result.content + .map((content: CallToolResult['content'][0]) => + content.type === 'text' ? content.text : JSON.stringify(content) + ) + .join('\n')}这样对 SDK 将来类型扩展也更健壮。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/adapter-gemini/package.jsonis excluded by!**/*.jsonpackages/extension-mcp/package.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
packages/extension-mcp/src/utils.ts(7 hunks)
🔇 Additional comments (3)
packages/extension-mcp/src/utils.ts (3)
45-56:isResourceReference逻辑保持正确,仅属性访问形式变化这里改成通过
resource['blob']/resource['text']判断为null/undefined,在运行时与原来的点访问等价,同时仍然只在存在uri且没有内联blob/text时才视为“资源引用”,符合 MCP 语义,看起来没有问题。
100-135:_toolOutputToContentBlocks重载参数类型收紧是正向改动将
content的类型改为CallToolResult['content'][0]能更精确约束调用方传入的是单个内容块,与下面result.content.map(...)的使用方式一致,有助于提升类型安全性。重载返回类型仍然根据useStandardContentBlocks字面量布尔精确区分,设计合理。
391-395: I'm unable to access the ChatLunaLab/chatluna repository through available channels (both direct cloning and GitHub raw content fetch are failing). This prevents me from verifying the actual code implementation, type signatures, and usage patterns needed to validate the review comment.To proceed with verification, I would need:
Direct file contents: The actual code from
packages/extension-mcp/src/utils.tsshowing:
- The complete
_convertCallToolResultfunction definition and its return type signature- The
callToolfunction that uses this result- How the return value is consumed downstream
Type definitions: Any interface or type definitions for the return values
Usage context: How callers handle the returned values
Could you provide either:
- The file contents or relevant code snippets from
packages/extension-mcp/src/utils.ts, or- Access to the repository so I can retrieve these details?
Once I have this information, I can verify whether the type mismatch is a real contract violation or if it's already handled correctly in practice.
This PR adds MCP client reconnection capabilities and fixes Gemini adapter issues.
New Features
Bug Fixes
Other Changes