refactor: mcp server helper #321
Conversation
|
""" WalkthroughThe MCPControllerRegister class is refactored to delegate all MCP server registration logic to new MCPServerHelper instances, removing its own registration methods and direct server management. A new MCPServerHelper class is introduced to encapsulate the registration of prompts, tools, and resources, centralizing initialization and control flow for MCP server interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant MCPControllerRegister
participant MCPServerHelper
participant McpServer
participant Controller
App->>MCPControllerRegister: register()
MCPControllerRegister->>MCPServerHelper: mcpPromptRegister / mcpToolRegister / mcpResourceRegister
MCPServerHelper->>McpServer: register prompt/tool/resource
McpServer->>Controller: invoke method with prepared args
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
plugin/controller/test/mcp/helper.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. plugin/controller/lib/impl/mcp/MCPControllerRegister.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
118-146:⚠️ Potential issue
statelessMcpServerHelpermay beundefinedandawaitis missing
connectStatelessStreamTransport()isasync, butstatelessMcpServerHelper.server.connect(...)is not awaited, and the helper itself is only instantiated later insideregister().
If this static method is ever invoked beforeregister(), the code will throwTypeError: Cannot read properties of undefined. A defensive check plus anawaitsolves both issues:- MCPControllerRegister.instance.statelessMcpServerHelper.server.connect( - MCPControllerRegister.instance.statelessTransport - ); + const helper = MCPControllerRegister.instance.statelessMcpServerHelper; + if (!helper) { + throw new Error('statelessMcpServerHelper has not been initialised. Call register() first.'); + } + await helper.server.connect(MCPControllerRegister.instance.statelessTransport);🧰 Tools
🪛 Biome (1.9.4)
[error] 118-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
♻️ Duplicate comments (1)
plugin/controller/lib/impl/mcp/MCPServerHelper.ts (1)
158-173: Same duplication bug as inmcpToolRegister
mcpPromptRegisterrepeats the exactnewArgs = [...newArgs, ...args]logic. Apply the same fix here to prevent duplicated parameter lists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(6 hunks)plugin/controller/lib/impl/mcp/MCPServerHelper.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
489-499: Asynchronous registration calls are now properly awaitedGood improvement! All the registration calls are now properly awaited, which fixes the potential race condition issue identified in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(6 hunks)plugin/controller/lib/impl/mcp/MCPServerHelper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/lib/impl/mcp/MCPServerHelper.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (5)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (5)
29-29: LGTM! Good addition of the MCPServerHelper import.The import of MCPServerHelper is correctly added to support the refactoring.
78-79: LGTM! Refactoring to use helper classes.The addition of these helper properties is a good architectural improvement. It encapsulates MCP server functionality and follows the single responsibility principle.
269-269: LGTM! Connect method is properly awaited.Good use of
awaitfor the asynchronous connection operation.
360-360: LGTM! Connect method is properly awaited.Good use of
awaitfor the asynchronous connection operation.
467-482: LGTM! Proper initialization of helper classes.The initialization of helpers is well-structured and only happens once. Good practice to include both name and version from the controller metadata, with fallbacks to sensible defaults.
b0819b7 to
b831d4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts ( 10000 3)
119-119: Connect method is now properly awaitedGood fix! The async connect method is now properly awaited, which addresses a potential race condition mentioned in a previous review.
489-491: Asynchronous registration calls are now properly awaitedGreat improvement! The asynchronous registration calls are now properly awaited, which prevents potential race conditions where the server might start handling requests before registration completes.
466-466:⚠️ Potential issueMethod signature should be updated to match implementation
The
register()method performs async operations but isn't marked asasync. This creates a disconnect between the method signature and its implementation.- async register() { + async register() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(6 hunks)plugin/controller/lib/impl/mcp/MCPServerHelper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/lib/impl/mcp/MCPServerHelper.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
🔇 Additional comments (7)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (7)
29-29: New dependency imported for the refactorGood addition of the
MCPServerHelperimport to support the delegation pattern implemented in this refactor.
78-79: Public properties added for MCPServerHelper instancesExposing these helpers as public properties makes the implementation more transparent and follows the delegation pattern correctly.
269-269: Connect method properly awaitedThe connect method is now correctly awaited, which prevents potential race conditions when setting up the transport.
360-360: Connect method properly awaitedThe connect method is now correctly awaited, which prevents potential race conditions when setting up the transport.
467-482: Good initialization of MCPServerHelper instancesThe initialization logic properly creates the helper instances with appropriate configuration and only when needed. It also correctly initializes the MCP servers.
493-495: Asynchronous registration calls are now properly awaitedThe asynchronous resource registration calls are now properly awaited, preventing race conditions.
497-499: Asynchronous registration calls are now properly awaitedThe asynchronous tool registration calls are now properly awaited, preventing race conditions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
483-502: Consider adding error handling for registration failuresThe registration process delegates to helper methods but lacks proper error handling. If any registration fails, the error will propagate up and could potentially disrupt the entire registration process.
Consider adding try-catch blocks to handle potential registration failures:
for (const proto of this.controllerProtos) { if (this.registeredControllerProtos.includes(proto)) { continue; } const metadata = proto.getMetaData(CONTROLLER_META_DATA) as MCPControllerMeta; + try { for (const prompt of metadata.prompts) { await this.mcpServerHelper.mcpPromptRegister(this.eggContainerFactory.getOrCreateEggObject.bind(this), proto, prompt); await this.statelessMcpServerHelper.mcpPromptRegister(this.eggContainerFactory.getOrCreateEggObject.bind(this), proto, prompt); } for (const resource of metadata.resources) { await this.mcpServerHelper.mcpResourceRegister(this.eggContainerFactory.getOrCreateEggObject.bind(this), proto, resource); await this.statelessMcpServerHelper.mcpResourceRegister(this.eggContainerFactory.getOrCreateEggObject.bind(this), proto, resource); } for (const tool of metadata.tools) { await this.mcpServerHelper.mcpToolRegister(this.eggContainerFactory.getOrCreateEggObject.bind(this), proto, tool); await this.statelessMcpServerHelper.mcpToolRegister(this.eggContainerFactory.getOrCreateEggObject.bind(this), proto, tool); } this.registeredControllerProtos.push(proto); + } catch (error) { + this.app.logger.error('Failed to register controller %s: %o', proto.name, error); + // Optionally, consider whether to throw or continue with registration of other controllers + } }
🧹 Nitpick comments (4)
plugin/controller/test/mcp/helper.test.ts (4)
13-18: Consider adding more descriptive test namesWhile the current test name works, consider adding more descriptive test names that explain what specific aspects of the MCPServerHelper you're testing.
-describe('plugin/controller/test/mcp/mcp.test.ts', () => { - it('MCPServerHelper should work', async () => { +describe('MCPServerHelper', () => { + it('should register and handle prompts, tools, and resources correctly', async () => {
58-92: Consider separating mock object creation into a helper functionThe mock object creation could be extracted into a helper function to improve readability and maintainability.
+const createMockEggObject = (promptMeta, toolMeta, resourceMeta) => ({ obj: { + [promptMeta.name]: args => { + return { + messages: [ + { + role: 'user', + content: { + type: 'text', + text: `Generate a concise but descriptive commit message for these changes:\n\n${args.name}`, + }, + }, + ], + }; + }, + [toolMeta.name]: args => { + return { + content: [ + { + type: 'text', + text: `npm package: ${args.name} not found`, + }, + ], + }; + }, + [resourceMeta.name]: uri => { + return { + contents: [ + { + uri: uri.toString(), + text: 'MOCK TEXT', + }, + ], + }; + }, +} }); + +// Then use it in the test +const getOrCreateEggObject = () => createMockEggObject(promptMeta, toolMeta, resourceMeta);
94-96: Avoid using type assertions in testsUsing
as anytype assertions can hide type errors and reduce type safety. Consider creating proper mock objects that match the expected types.-await helper.mcpToolRegister(getOrCreateEggObject as any, { getMetaData: () => ({}) } as any, toolMeta); +// Create a properly typed mock +const typedProto = { + getMetaData: () => ({}), + name: 'mockedProto' + // Add other required properties of EggPrototype +}; +await helper.mcpToolRegister(getOrCreateEggObject, typedProto, toolMeta);
171-173: Add test for error handling scenariosThe test covers the happy path but doesn't test any error scenarios. Consider adding tests for how the helper handles registration failures or invalid requests.
+ it('should handle registration errors correctly', async () => { + const helper = new MCPServerHelper({ + name: 'test-errors', + version: '1.0.0', + hooks: [], + }); + + // Test with invalid metadata + const invalidResourceMeta = new MCPResourceMeta({ + name: 'invalidResource', + needAcl: false, + middlewares: [], + // Missing both uri and template should cause an error + }); + + try { + await helper.mcpResourceRegister(() => ({ obj: {} }), { getMetaData: () => ({}) } as any, invalidResourceMeta); + assert.fail('Should have thrown an error'); + } catch (error) { + assert.ok(error.message.includes('must have uri or template')); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(6 hunks)plugin/controller/package.json(1 hunks)plugin/controller/test/mcp/helper.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugin/controller/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (6)
core/runtime/src/factory/EggContainerFactory.ts (1)
EggContainerFactory(18-88)core/types/metadata/model/EggPrototype.ts (1)
EggPrototype(117-154)plugin/controller/lib/impl/mcp/MCPServerHelper.ts (1)
MCPServerHelper(20-184)core/metadata/src/model/graph/GlobalGraph.ts (1)
proto(143-159)core/types/controller-decorator/MetadataKey.ts (1)
CONTROLLER_META_DATA(9-9)core/controller-decorator/src/model/MCPControllerMeta.ts (1)
MCPControllerMeta(7-70)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (14)
plugin/controller/test/mcp/helper.test.ts (7)
1-12: LGTM: Well-structured test imports and setupThe imports are properly organized, bringing in the necessary components for testing the MCPServerHelper class.
25-29: LGTM: Clean MCPServerHelper instantiationClean instantiation of the MCPServerHelper with proper configuration parameters.
31-56: LGTM: Proper MCP metadata setupThe test correctly sets up the necessary metadata for resources, tools, and prompts with appropriate configurations.
98-115: LGTM: Proper client-server transport setupThe test correctly sets up the in-memory transport pair and connects both client and server.
116-133: LGTM: Thorough tool registration and invocation testingThe test validates both listing of tools and calling tools with arguments, with proper assertions.
134-151: LGTM: Complete resource and prompt testingThe test covers listing and reading resources as well as listing prompts with their argument metadata.
153-169: LGTM: Proper prompt invocation testingThe test correctly validates prompt invocation and the expected response format.
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (7)
29-29: LGTM: MCPServerHelper import addedThe import of MCPServerHelper is appropriate for the refactoring.
72-80: Server implementation delegated to helper classesThe refactoring correctly moves the server management to dedicated helper classes, improving separation of concerns.
119-119: Connect method is properly awaitedGood job addressing the previous comment by ensuring the connect method is awaited, preventing potential race conditions.
269-269: Server reference correctly updatedThe code now properly uses the server property from the helper instance.
360-360: Server reference correctly updatedThe code now properly uses the server property from the helper instance.
467-482: LGTM: Helper instances initializationThe helper instances are properly initialized with appropriate configuration parameters.
488-499: Asynchronous registration calls are properly awaitedGood job addressing the previous comment by properly awaiting the registration calls to prevent race conditions.
dc2e31e to
0e113b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
483-501: Consider add 8000 ing error handling for registration failuresThe registration process delegates to helper methods but lacks proper error handling. If any registration fails, the error will propagate up and could potentially disrupt the entire registration process.
🧹 Nitpick comments (4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (4)
466-466: Add explicit async declaration to the register methodThe register method contains await statements but isn't explicitly marked as async. While TypeScript automatically returns a Promise when await is used, explicit declaration improves code clarity.
- async register() { + async register(): Promise<void> {
467-482: Initialization logic could be more robustThe initialization only checks if
mcpServerHelperexists but doesn't verifystatelessMcpServerHelper. While they're always initialized together in the current implementation, a more robust check would verify both instances.- if (!this.mcpServerHelper) { + if (!this.mcpServerHelper || !this.statelessMcpServerHelper) {
489-490: Optimize repetitive binding operationsThe binding of
getOrCreateEggObjectis repeated within each loop iteration. It would be more efficient to create this bound function once before the loops.async register() { if (!this.mcpServerHelper) { this.mcpServerHelper = new MCPServerHelper({ name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, version: this.controllerMeta.version ?? '1.0.0', hooks: MCPControllerRegister.hooks, }); this.statelessMcpServerHelper = new MCPServerHelper({ name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, version: this.controllerMeta.version ?? '1.0.0', hooks: MCPControllerRegister.hooks, }); this.mcpStatelessStreamServerInit(); this.mcpStreamServerInit(); this.mcpServerInit(); this.mcpServerRegister(); } + const getOrCreateObject = this.eggContainerFactory.getOrCreateEggObject.bind(this.eggContainerFactory); for (const proto of this.controllerProtos) { if (this.registeredControllerProtos.includes(proto)) { continue; } const metadata = proto.getMetaData(CONTROLLER_META_DATA) as MCPControllerMeta; for (const prompt of metadata.prompts) { - await this.mcpServerHelper.mcpPromptRegister(this.eggContainerFactory.getOrCreateEggObject.bind(this.eggContainerFactory), proto, prompt); - await this.statelessMcpServerHelper.mcpPromptRegister(this.eggContainerFactory.getOrCreateEggObject.bind(this.eggContainerFactory), proto, prompt); + await this.mcpServerHelper.mcpPromptRegister(getOrCreateObject, proto, prompt); + await this.statelessMcpServerHelper.mcpPromptRegister(getOrCreateObject, proto, prompt); } // Apply the same pattern to the other loops
29-29: Consider adding export for MCPServerHelperThe MCPServerHelper is only imported but not re-exported from this file. If this is meant to be used by external code, consider adding an export.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(6 hunks)plugin/controller/package.json(1 hunks)plugin/controller/test/mcp/helper.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/controller/package.json
- plugin/controller/test/mcp/helper.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (20)
🔇 Additional comments (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
119-119: LGTM! Connect method is now properly awaitedExcellent! I see you've fixed the issue from the previous review by properly awaiting the connect method. This prevents potential race conditions.
72-79: Good refactoring approach using helper classesThe refactoring to use helper classes improves separation of concerns by moving server management to dedicated
MCPServerHelperinstances instead of handling everything in the register class.
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
No changes to user-facing features or functionality.