[Fix] validate routing and stabilize skill reloads#859
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (17)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough该 PR 添加 chatMode 验证并映射对应错误，修正代理触发器的 bindingKey 与 conversationId 合并/路由逻辑，增量化内置技能同步并改进热重载调度，规范化空工具调用 ID，为运行时同步日志添加条件输出。 变更聊天模式验证系统
代理触发器对话和绑定路由
工具调用 ID 规范化
内置技能增量同步与热重载
估计代码审查工作量🎯 4 (Complex) | ⏱️ ~45 分钟 可能相关的 PR
诗歌
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/services/conversation.ts (1)
1756-1765: ⚡ Quick win建议内联此验证逻辑以遵循编码规范。
根据编码规范，简短逻辑（1-5行）不应提取为独立函数。虽然此方法被调用了4次，但验证逻辑非常简单（仅一个条件检查和抛出错误），应该在每个调用点直接内联。这样可以减少抽象层次，使代码更加直接。
♻️ 建议的内联重构
删除
checkChatMode方法，在每个调用点内联验证：在
createConversation(608行附近):- this.checkChatMode(options.chatMode) + if ( + options.chatMode != null && + !this.ctx.chatluna.platform.chatChains.value.some( + (chain) => chain.name === options.chatMode + ) + ) { + throw new Error(`Chat mode ${options.chatMode} not found.`) + }在
updateConversationUsage(1566行附近):- this.checkChatMode(options.chatMode) + if ( + options.chatMode != null && + !this.ctx.chatluna.platform.chatChains.value.some( + (chain) => chain.name === options.chatMode + ) + ) { + throw new Error(`Chat mode ${options.chatMode} not found.`) + }在
updateManagedConstraint(1697行附近):- this.checkChatMode(patch.defaultChatMode) - this.checkChatMode(patch.fixedChatMode) + if ( + patch.defaultChatMode != null && + !this.ctx.chatluna.platform.chatChains.value.some( + (chain) => chain.name === patch.defaultChatMode + ) + ) { + throw new Error(`Chat mode ${patch.defaultChatMode} not found.`) + } + if ( + patch.fixedChatMode != null && + !this.ctx.chatluna.platform.chatChains.value.some( + (chain) => chain.name === patch.fixedChatMode + ) + ) { + throw new Error(`Chat mode ${patch.fixedChatMode} not found.`) + }然后删除
checkChatMode方法定义（1756-1765行）。编码规范规定："如果函数体只有1-5行，请在调用点内联它，而不是提取成独立函数"以及"编写最简单的代码，减少抽象、函数和变量"。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/services/conversation.ts` around lines 1756 - 1765, The short validation in checkChatMode should be inlined at each call site instead of kept as a separate method: replace calls to checkChatMode(mode) in createConversation, updateConversationUsage, and updateManagedConstraint with the equivalent inline guard (if mode != null && !this.ctx.chatluna.platform.chatChains.value.some(chain => chain.name === mode) throw new Error(`Chat mode ${mode} not found.`)); after inlining at all call sites, remove the private checkChatMode method definition entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/services/conversation.ts`:
- Around line 1756-1765: The short validation in checkChatMode should be inlined
at each call site instead of kept as a separate method: replace calls to
checkChatMode(mode) in createConversation, updateConversationUsage, and
updateManagedConstraint with the equivalent inline guard (if mode != null &&
!this.ctx.chatluna.platform.chatChains.value.some(chain => chain.name === mode)
throw new Error(`Chat mode ${mode} not found.`)); after inlining at all call
sites, remove the private checkChatMode method definition entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 379d8269-ddca-4cf4-b740-5286655c6885
⛔ Files ignored due to path filters (2)
packages/core/src/locales/en-US.ymlis excluded by!**/*.ymlpackages/core/src/locales/zh-CN.ymlis excluded by!**/*.yml
📒 Files selected for processing (5)
packages/core/src/middlewares/system/conversation_manage.tspackages/core/src/services/conversation.tspackages/extension-agent/src/service/trigger.tspackages/extension-agent/src/trigger/executor.tspackages/shared-adapter/src/utils.ts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces chat mode validation across the conversation services, including localized error messages for invalid modes. It also refines the conversation trigger logic to handle binding key changes and resets conversation IDs appropriately. Additionally, it ensures that empty tool call IDs are treated as undefined in the shared adapter. Feedback was provided to improve the explicitness of property checks when updating conversation IDs in the trigger service.
dingyi222666
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/extension-agent/src/skills/watch.ts (1)
39-42: 💤 Low value冗余的
reloading二次判断可移除。外层
schedule()（Lines 32-35）已在reloading为 true 时直接返回，不会再注册新的定时器；而reloading = true只在这同一个定时器回调里被设置（Line 44），因此当定时器真正触发时reloading一定为 false。这里的二次检查属于防御性死代码。As per coding guidelines: "Do not add type guards, null checks, or fallback values unless there is concrete evidence that the value can actually be null/undefined/wrong-type".♻️ 建议移除冗余分支
timer = setTimeout(async () => { timer = undefined - if (reloading) { - pending = true - return - } - reloading = true try { await reload()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/extension-agent/src/skills/watch.ts` around lines 39 - 42, The inner defensive check that tests the module-level reloading flag inside the timer callback is redundant; remove the block that checks reloading and sets pending (the if (reloading) { pending = true; return } inside the schedule/timer callback) since schedule() already returns early when reloading is true and reloading is only set within the same timer callback; update the schedule()/timer callback in watch.ts to drop that branch and rely on the existing outer guard, leaving pending handling only where actually needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/extension-agent/src/skills/watch.ts`:
- Around line 39-42: The inner defensive check that tests the module-level
reloading flag inside the timer callback is redundant; remove the block that
checks reloading and sets pending (the if (reloading) { pending = true; return }
inside the schedule/timer callback) since schedule() already returns early when
reloading is true and reloading is only set within the same timer callback;
update the schedule()/timer callback in watch.ts to drop that branch and rely on
the existing outer guard, leaving pending handling only where actually needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 71b04d57-dd08-466f-a2a3-52f95591da9d
📒 Files selected for processing (2)
packages/extension-agent/src/skills/builtin.tspackages/extension-agent/src/skills/watch.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pr fixes conversation chat mode validation, trigger conversation routing, empty tool-call IDs, and bundled skill reload behavior so invalid configuration fails clearly and skill updates avoid unnecessary reload loops.
Fixes #858
Fixes #860
New Features
None
Bug fixes
newConversationis true and no conversation ID is already provided.config.jsonfiles.Other Changes
None