-
-
Notifications
You must be signed in to change notification settings - Fork 41
fix(extension-mcp,extension-tools): improve MCP client initialization and tool selector logic #581
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
…est tools
Add requestHeaders configuration to allow setting custom headers based on domain matching patterns. This enables different headers to be applied to different domains when using the web_fetcher and web_poster tools.
- Add requestHeaders config array with matcher and headers fields
- Implement getHeadersForUrl() function using micromatch for domain pattern matching
- Update RequestsGetTool and RequestsPostTool to support headerConfigs
- Add i18n translations for new configuration options in zh-CN and en-US
- Headers are merged with default headers, with matched headers taking precedence
Example usage:
requestHeaders:
- matcher: "api.github.com"
headers:
Authorization: "Bearer token123"
- matcher: "*.example.com"
headers:
X-Custom-Header: "value"
… and tool selector logic - Move ChatLunaPlugin instance creation to module scope to avoid initialization issues - Upgrade @modelcontextprotocol/sdk to 1.19.1 and remove unused json-schema-to-zod dependency - Add client metadata (title, description) for better MCP protocol compliance - Improve logging messages for better clarity during initialization and connection - Fix prepareClient to return true only when at least one server connects successfully - Enhance cron tool selector to check all history messages instead of only the last one - Add proper type annotation for fs tool selector - Remove redundant empty lines in fs tool registrations
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 by dingyi222666 focuses on enhancing the MCP client's initialization and tool selection mechanisms, alongside introducing domain-specific header configurations for request tools. It includes dependency updates, significant refactoring for robustness, and improvements to tool activation logic. 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
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (26)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 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该变更引入共享的 ChatLunaPlugin 实例并调整 MCP 客户端就绪逻辑(至少一台服务器可用);为请求工具新增按域匹配的请求头配置并在 GET/POST 中合并;cron 选择器改为扫描整个历史;强化 fs 工具类型注解;新增消息转发长度阈值与文本提取逻辑。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Koishi as Koishi Context
participant Plugin as ChatLunaPlugin (shared)
participant MCP as MCP Client
participant Servers as MCP Servers
User->>Koishi: 启动 extension-mcp
Koishi->>Plugin: new ChatLunaPlugin(ctx, config, "mcp-client", false)
Koishi->>MCP: 初始化 (title, description)
loop 连接各服务器
MCP->>Servers: 尝试连接
alt 连接成功
MCP-->>MCP: availableServers += 1
else 连接失败
MCP-->>MCP: 记录失败日志
end
end
alt availableServers >= 1
MCP-->>Koishi: 就绪(注册/工具准备)
else
MCP-->>Koishi: 未就绪(无可用服务器)
end
sequenceDiagram
autonumber
actor Agent
participant Tool as RequestsGetTool / RequestsPostTool
participant Matcher as Header Matcher (micromatch)
participant HTTP as HTTP Client
Agent->>Tool: 调用 (url, options.headers)
Tool->>Matcher: getHeadersForUrl(url, headerConfigs)
Matcher-->>Tool: matchedHeaders (可能为空)
Tool-->>Tool: 合并 headers = {...options.headers, ...matchedHeaders}
Tool->>HTTP: 发起请求 (url, headers)
HTTP-->>Tool: 响应
Tool-->>Agent: 返回结果
note right of Matcher: 使用 hostname 与 matcher 列表匹配
sequenceDiagram
autonumber
actor System
participant Sender as DefaultChatChainSender
participant Config as Core Config
System->>Sender: 请求转发(msgs, isForwardMsg)
Sender->>Sender: text = getMessageText(msgs)
Sender->>Config: 检查 forwardMsgMinLength
alt isForwardMsg && text.length >= forwardMsgMinLength
Sender->>System: 执行转发
else
Sender->>System: 跳过转发
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
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 some great improvements, particularly the domain-based header configuration for request tools and the more robust MCP client initialization. The tool selector logic enhancement to check the entire history is also a valuable change.
I've identified a critical issue regarding the ChatLunaPlugin initialization that needs to be addressed, as well as a medium-severity issue concerning a potentially breaking change in a tool's name. Please see my detailed comments below.
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: 1
🧹 Nitpick comments (2)
packages/extension-tools/src/plugins/request.ts (2)
12-31: 整体实现正确,建议增强错误可观测性。
getHeadersForUrl函数的实现逻辑正确:
- 使用
micromatch进行模式匹配,{ dot: true }选项正确处理子域名- 返回第一个匹配的配置,符合预期
- 错误处理防止无效 URL 导致崩溃
不过,静默捕获所有错误可能会隐藏问题。建议在生产环境中记录解析失败的情况。
可选的改进建议:
} catch (error) { - // Invalid URL, return empty headers + // Invalid URL, return empty headers + // 可以考虑添加日志记录以便调试 + // console.warn(`Failed to parse URL for header matching: ${url}`, error) }
64-76: 改进了空值处理的健壮性。使用
(config.requestSelector?.length ?? 0) === 0替代之前的> 0检查,更明确地处理了undefined情况。不过第 73 行的
config?.requestSelector || []有些冗余,因为上面已经检查过长度。但这种防御性编程也无害。可选的简化建议:
- config?.requestSelector || [] + config.requestSelector!因为在第 65 行已经确保了
requestSelector存在且长度大于 0。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/extension-mcp/package.jsonis excluded by!**/*.jsonpackages/extension-tools/src/locales/en-US.schema.ymlis excluded by!**/*.ymlpackages/extension-tools/src/locales/zh-CN.schema.ymlis excluded by!**/*.yml
📒 Files selected for processing (6)
packages/extension-mcp/src/index.ts(2 hunks)packages/extension-mcp/src/service.ts(7 hunks)packages/extension-tools/src/config.ts(2 hunks)packages/extension-tools/src/plugins/cron.ts(2 hunks)packages/extension-tools/src/plugins/fs.ts(2 hunks)packages/extension-tools/src/plugins/request.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/extension-tools/src/plugins/cron.ts (1)
packages/core/src/utils/string.ts (2)
fuzzyQuery(29-37)getMessageContent(150-166)
packages/extension-tools/src/plugins/fs.ts (1)
packages/core/src/llm-core/platform/types.ts (1)
ChatLunaTool(62-68)
packages/extension-mcp/src/index.ts (1)
packages/core/src/services/chat.ts (1)
ChatLunaPlugin(594-792)
packages/extension-tools/src/plugins/request.ts (2)
packages/core/src/utils/string.ts (2)
fuzzyQuery(29-37)getMessageContent(150-166)packages/core/src/services/chat.ts (1)
ChatLunaPlugin(594-792)
packages/extension-mcp/src/service.ts (2)
packages/core/src/services/chat.ts (1)
ChatLunaPlugin(594-792)packages/extension-mcp/src/index.ts (2)
plugin(9-9)logger(8-8)
⏰ 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 (4)
packages/extension-tools/src/plugins/request.ts (4)
42-62: LGTM!工具实例化正确传递了
headerConfigs,使用空合并运算符??安全处理可选配置项。
93-97: LGTM!
RequestTool接口和具体实现类正确添加了headerConfigs属性,类型定义准确,初始化为空数组是合理的默认值。Also applies to: 114-114, 171-171
152-152: 确认破坏性更改及迁移支持
- 工具名称已从
web_post改为web_poster,请手动核查所有外部引用(配置、文档、CI 脚本等)已同步更新。- 如仍有用户或集成依赖旧名称,需提供迁移指南或对旧名称做兼容层。
- 发布说明中必须明确标注此破坏性变更。
139-142: 验证请求头合并顺序 当前合并为{ ...this.headers, ...matchedHeaders },matchedHeaders 会覆盖默认 headers;未在代码库中发现相关测试或文档,请确认该行为是否符合预期。
Add forwardMsgMinLength configuration option to control when messages should be sent as forward/merged messages. When set to 0, always use forward messages. When set to a value greater than 0, only messages exceeding that length will be sent as forward messages. Changes: - Add forwardMsgMinLength config option (0-400 range) in config.ts - Implement length check logic in DefaultChatChainSender - Add getMessageText helper method to calculate total message length - Update localization files for both en-US and zh-CN close #580
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/core/src/locales/en-US.schema.ymlis excluded by!**/*.ymlpackages/core/src/locales/zh-CN.schema.ymlis excluded by!**/*.yml
📒 Files selected for processing (2)
packages/core/src/chains/chain.ts(2 hunks)packages/core/src/config.ts(2 hunks)
⏰ 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 (2)
packages/core/src/config.ts (1)
9-9: 配置添加正确。新增的
forwardMsgMinLength配置字段类型定义和验证规则都正确。默认值为 0 表示当isForwardMsg启用时转发所有非空消息。Also applies to: 69-69
packages/core/src/chains/chain.ts (1)
723-727: 验证比较运算符是否符合预期。当前使用
>比较运算符,这意味着当forwardMsgMinLength设置为 10 时,只有长度 大于 10(即 11 及以上)的消息才会被转发。如果配置项语义是"最小长度",用户可能期望长度等于或大于 10 的消息被转发,这种情况下应使用>=运算符。请确认以下行为是否符合预期:
forwardMsgMinLength = 0(默认值):转发所有长度 ≥ 1 的消息 ✓forwardMsgMinLength = 10:仅转发长度 ≥ 11 的消息(而非 ≥ 10)如果"最小长度"应该表示"至少此长度",请应用以下修复:
if ( this.config.isForwardMsg && - this.getMessageText(messages).length > + this.getMessageText(messages).length >= this.config.forwardMsgMinLength ) {
Update core package version and all peer dependencies across the monorepo to version 1.3.0-alpha.54. Affected packages: - core: 1.3.0-alpha.53 -> 1.3.0-alpha.54 - extension-mcp: 1.3.0-alpha.5 -> 1.3.0-alpha.6 - extension-tools: 1.3.0-alpha.15 -> 1.3.0-alpha.16 - All adapter and service packages peer dependency updated
This PR improves the MCP (Model Context Protocol) client initialization and tool selector logic, along with domain-based header configuration for request tools.
Bug Fixes
New Features
Other Changes