Conversation
WalkthroughThis change introduces comprehensive support for the Model Context Protocol (MCP) as a new controller type within the Egg.js framework. It adds MCP-specific decorators, metadata builders, and utility functions for defining MCP controllers, prompts, tools, and resources. The plugin infrastructure is extended to register and manage MCP controllers, including their lifecycle, routing, and transport mechanisms (SSE and streamable HTTP). Middleware and configuration updates ensure correct request handling and CSRF bypass for MCP routes. Extensive type definitions and test suites are included to validate MCP integration. Additionally, a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPProxyApiClient
participant EggApp
participant MCPControllerRegister
participant MCPServer
Client->>MCPProxyApiClient: Sends MCP message (SSE/STREAM/STDIO)
MCPProxyApiClient->>EggApp: Proxies HTTP request (with protocol/session)
EggApp->>MCPControllerRegister: Handles MCP route (init/message/stream)
MCPControllerRegister->>MCPServer: Registers/dispatches prompt/tool/resource
MCPServer-->>MCPControllerRegister: Invokes controller method
MCPControllerRegister-->>EggApp: Returns response
EggApp-->>MCPProxyApiClient: Forwards response
MCPProxyApiClient-->>Client: Delivers response/event
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
core/controller-decorator/index.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. core/controller-decorator/src/decorator/mcp/Extra.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. core/controller-decorator/src/decorator/mcp/MCPController.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.
✨ 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:
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (1)
plugin/mcp-proxy/CHANGELOG.md (1)
1-927:⚠️ Potential issuePackage name mismatch in CHANGELOG.md
The CHANGELOG.md refers to
@eggjs/tegg-aop-pluginthroughout the document, but this file is located in theplugin/mcp-proxydirectory. This suggests the changelog was either copied from another package or the package name needs to be updated to reflect its actual location.Update the package name references to match the actual package (
@eggjs/mcp-proxy).-# [3.53.0](https://github.com/eggjs/tegg/compare/v3.52.1...v3.53.0) (2025-04-21) - -**Note:** Version bump only for package @eggjs/tegg-aop-plugin +# [3.53.0](https://github.com/eggjs/tegg/compare/v3.52.1...v3.53.0) (2025-04-21) + +**Note:** Version bump only for package @eggjs/mcp-proxyThis change should be applied throughout the entire changelog for consistency.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
356-356: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
653-653: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
664-664: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
819-819: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
830-830: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
849-849: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
924-924: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
🧹 Nitpick comments (42)
.eslintrc (1)
12-12: Disabled unresolved imports check—ensure it's intentional.Turning off
"import/no-unresolved"suppresses unresolved import errors, likely to accommodate new MCP modules. Confirm that this does not mask unintended import issues.Consider configuring the
import/resolverinsettings(e.g.,eslint-import-resolver-typescript) to handle TypeScript path mappings instead of disabling the rule entirely.plugin/controller/test/fixtures/apps/mcp-app/config/config.default.js (1)
1-1: Remove redundant 'use strict' directiveJavaScript modules automatically run in strict mode, making this directive unnecessary.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
plugin/mcp-proxy/app/extend/agent.ts (1)
9-13: Consider extracting type casting to improve readabilityThe repeated type casting of
thistoApplicationmakes the code less readable. Consider extracting this to a local variable.- this[MCP_PROXY] = new MCPProxyApiClient({ - logger: (this as unknown as Application).logger, - messenger: (this as unknown as Application).messenger, - app: this as unknown as Application, + const app = this as unknown as Application; + this[MCP_PROXY] = new MCPProxyApiClient({ + logger: app.logger, + messenger: app.messenger, + app, });plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/config.default.js (1)
1-1: Remove redundant 'use strict' directiveThe 'use strict' directive is redundant in ES modules as they are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
plugin/mcp-proxy/app/extend/application.ts (1)
8-12: Consider extracting type casting to improve readabilityThe repeated type casting of
thistoApplicationmakes the code less readable. Consider extracting this to a local variable.- this[MCP_PROXY] = new MCPProxyApiClient({ - logger: (this as unknown as Application).logger, - messenger: (this as unknown as Application).messenger, - app: this as unknown as Application, + const app = this as unknown as Application; + this[MCP_PROXY] = new MCPProxyApiClient({ + logger: app.logger, + messenger: app.messenger, + app, });plugin/controller/app/middleware/mcp_body_middleware.ts (1)
7-12: Consider optimizing path matching performanceThe current implementation compiles regular expressions for each path on every request. Consider pre-compiling these regular expressions outside the middleware function for better performance.
+const pathMatchers = ['/mcp/init', '/mcp/message', '/mcp/stream'].map(igPath => + pathToRegexp(igPath, [], { end: false }) +); + export default () => { return async function mcpBodyMiddleware(ctx: EggContext, next: Next) { - const arr = [ '/mcp/init', '/mcp/message', '/mcp/stream' ]; - const res = arr.some(igPath => { - const match = pathToRegexp(igPath, [], { - end: false, - }); - return match.test(ctx.path); - }); + const res = pathMatchers.some(matcher => matcher.test(ctx.path)); if (res) { ctx.disableBodyParser = true; }core/controller-decorator/src/decorator/mcp/MCPResource.ts (1)
9-32: The MCPResource decorator looks good, with one suggestion about parameter naming.The MCPResource decorator properly manages metadata for MCP resource methods, setting the controller type and storing resource parameters. The code is well-structured and follows the decorator pattern correctly.
There's a potential naming conflict in how you're handling the
nameproperty:- mcpName: params.name, - name: methodName, + resourceName: methodName,While the current implementation works, using
namefor the method name andmcpNamefor the original name parameter could be confusing when accessing these values later. Consider using a more descriptive property name likeresourceNamefor the method name to improve code clarity.plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/router.ts (1)
1-7: Router configuration looks good, but could benefit from comments.The router configuration correctly sets up the necessary routes for the MCP proxy test application. The three endpoints (/stream, /init, /message) with their respective HTTP methods map to the appropriate controller methods.
Consider adding brief comments explaining the purpose of each route to improve maintainability:
module.exports = (app: Application) => { + // Stream endpoint for SSE or streamable HTTP transport app.router.all('/stream', app.controller.app.allStream); + // Initialize MCP server session app.router.get('/init', app.controller.app.init); + // Handle MCP message requests app.router.post('/message', app.controller.app.message); };core/controller-decorator/src/decorator/mcp/MCPTool.ts (1)
21-29: Consider adding a comment to clarify the name handlingThe implementation correctly sets the MCP tool parameters, but the distinction between
mcpName(fromparams?.name) andname(frommethodName) might be confusing for future developers. Consider adding a comment explaining why both fields are needed and how they differ.MCPInfoUtil.setMCPToolParams( { ...params, + // mcpName is an optional explicit name for the tool in MCP context + // while name defaults to the method name mcpName: params?.name, name: methodName, }, controllerClazz, methodName, );core/controller-decorator/src/impl/mcp/MCPControllerResourceMetaBuilder.ts (1)
1-35: Consider refactoring to reduce duplication with MCPControllerToolMetaBuilderThe implementation is correct, but there's significant code duplication with
MCPControllerToolMetaBuilder. Both classes share nearly identical logic, differing only in the metadata type they create and the specific parameter getter they use.Consider creating a base class or utility function to handle the common logic:
// Example base class approach abstract class MCPControllerBaseMetaBuilder<T> { protected readonly clazz: EggProtoImplClass; protected readonly methodName: string; constructor(clazz: EggProtoImplClass, methodName: string) { this.clazz = clazz; this.methodName = methodName; } protected abstract getParams(): any; protected abstract createMeta(params: any): T; build(): T | undefined { MethodValidator.validate(this.clazz, this.methodName); const controllerType = MethodInfoUtil.getMethodControllerType(this.clazz, this.methodName); if (!controllerType) { return undefined; } const middlewares = MethodInfoUtil.getMethodMiddlewares(this.clazz, this.methodName); const needAcl = MethodInfoUtil.hasMethodAcl(this.clazz, this.methodName); const aclCode = MethodInfoUtil.getMethodAcl(this.clazz, this.methodName); const params = this.getParams(); return this.createMeta({ name: this.methodName, middlewares, needAcl, aclCode, ...params, }); } } // Then implement specific builders export class MCPControllerResourceMetaBuilder extends MCPControllerBaseMetaBuilder<MCPResourceMeta> { protected getParams() { return MCPInfoUtil.getMCPResourceParams(this.clazz, this.methodName); } protected createMeta(params: any): MCPResourceMeta { return new MCPResourceMeta(params); } }core/controller-decorator/src/decorator/mcp/MCPPrompt.ts (1)
6-7: Consider simplifying the import paths.The relative import paths
../../../src/util/MCPInfoUtiland../../../src/util/MethodInfoUtilseem unnecessarily complex. Consider using more direct import paths if possible, or adjusting the directory structure to reduce path complexity.core/controller-decorator/src/decorator/mcp/MCPController.ts (3)
17-23: Remove commented-out code paths.Lines 17-22 contain commented-out file paths that appear to be debugging artifacts or notes. These should be removed for cleaner production code.
- // './tegg/core/common-util/src/StackUtil.ts', - // './tegg/core/core-decorator/src/decorator/Prototype.ts', - // './tegg/core/controller-decorator/src/decorator/tr/TRController.ts', - // './tegg/core/core-decorator/node_modules/[email protected]@reflect-metadata/Reflect.js', - // './tegg/core/core-decorator/node_modules/[email protected]@reflect-metadata/Reflect.js', - // './tegg/core/controller-decorator/test/fixtures/TRFooController.ts',
7-7: Avoid shadowing the global "constructor" property.The parameter name
constructorshadows the global JavaScriptconstructorproperty, which can lead to confusion. Consider renaming it to something more specific.- return function(constructor: EggProtoImplClass) { + return function(targetClass: EggProtoImplClass) {🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
23-23: Document the magic number in stack retrieval.Line 23 uses a magic number
5in theStackUtil.getCalleeFromStack(false, 5)call. It's not clear why that specific stack depth is chosen, which makes the code harder to maintain.- PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5)); + // Skip 5 frames: current function, decorator factory, reflect-metadata internals, and user code + const stackDepth = 5; + PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, stackDepth));core/controller-decorator/src/model/MCPToolMeta.ts (2)
3-4: Remove extra blank lines.There are two blank lines after the imports (lines 3-4). Consider removing one of them for consistent code style.
import type { MiddlewareFunc } from '@eggjs/tegg-types'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; -
12-12: Ensure array immutability.The
middlewaresproperty is markedreadonlybut this only makes the reference readonly, not the array contents. If full immutability is required, consider making a defensive copy in the constructor.export class MCPToolMeta { // ... readonly middlewares: readonly MiddlewareFunc[]; // ... constructor(opt: { // ... }) { // ... - this.middlewares = opt.middlewares; + // Create a defensive copy to ensure array contents can't be modified + this.middlewares = Object.freeze([...opt.middlewares]); // ... } }core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts (1)
24-25: Consider adding error handling for MCPInfoUtil.getMCPPromptParamsThe call to
MCPInfoUtil.getMCPPromptParams()assumes the operation will always succeed. Consider adding error handling to gracefully handle any potential errors in retrieving the MCP prompt parameters.core/controller-decorator/test/MCPMeta.test.ts (2)
11-23: Add descriptive messages to assertionsConsider adding descriptive messages to the assertions to make test failures more informative and easier to debug.
- assert(fooControllerMetaData); + assert(fooControllerMetaData, 'Controller metadata should exist'); - assert(fooControllerMetaData.type === ControllerType.MCP); + assert(fooControllerMetaData.type === ControllerType.MCP, 'Controller type should be MCP');Similarly for the other assertions.
7-25: Use a more descriptive test nameThe current test name "should work" doesn't clearly describe what functionality is being tested. Consider using a more descriptive name that explains the purpose of the test.
- it.only('should work', () => { + it('should correctly build MCP controller metadata', () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/plugin.js (2)
1-1: Consider removing redundant 'use strict' directiveJavaScript modules are automatically in strict mode, so the 'use strict' directive is redundant. However, if this follows a consistent pattern in other configuration files in the project, it's acceptable to keep it for consistency.
-'use strict'; // eslint-disable-next-line @typescript-eslint/no-var-requires🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
16-19: Path resolution could be more robustThe current path resolution uses a relative path with multiple parent directory references (
../../../../../). This approach can be fragile if the directory structure changes. Consider using a more robust way to locate the plugin.For test fixtures, you might consider using the actual package name instead of a relative path, or storing the plugin path in a shared test configuration.
plugin/mcp-proxy/lib/MCPProxyDataClient.ts (2)
21-30: Refactor to eliminate code duplication in registerClient methodThe
registerClientmethod duplicates thethis.clients.set(sessionId, pid)call in both the if and else branches.async registerClient(sessionId: string, pid: number) { if (this.clients.has(sessionId)) { const oldPid = this.clients.get(sessionId)!; this.logger.info('[MCPClientManager] duplicate register client %s new pid %s old pid', sessionId, pid, oldPid); - this.clients.set(sessionId, pid); } else { this.logger.info('[MCPClientManager] register client %s pid %s', sessionId, pid); - this.clients.set(sessionId, pid); } + this.clients.set(sessionId, pid); }
18-19: Consider removing empty init method if not neededThe
_initmethod is marked with an ESLint disable comment for an empty function. If this method is not actually needed for overriding a base class method, consider removing it entirely.- // eslint-disable-next-line @typescript-eslint/no-empty-function - async _init() {}If it's required by the Base class contract, add a comment explaining why this empty implementation is necessary.
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js (1)
1-2: Remove redundant 'use strict' directiveThe 'use strict' directive is redundant in JavaScript modules as they are automatically in strict mode.
-'use strict'; -🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts (2)
29-29: Fix spacing in parameter type annotationThere's an extra space before the type annotation colon.
- async startNotificationStream(args: ToolArgs<typeof NotificationType>, extra :ToolExtra): Promise<MCPToolResponse> { + async startNotificationStream(args: ToolArgs<typeof NotificationType>, extra: ToolExtra): Promise<MCPToolResponse> {
35-49: Consider adding a safety timeout for infinite loopsWhen
count === 0, the loop will run indefinitely. Consider adding a maximum runtime limit or a way to terminate the stream to prevent resource exhaustion in production environments.+ const MAX_RUNTIME_MS = 30 * 60 * 1000; // 30 minutes maximum runtime + const startTime = Date.now(); while (count === 0 || counter < count) { + // Safety check for infinite loops + if (count === 0 && (Date.now() - startTime) > MAX_RUNTIME_MS) { + console.log('Maximum runtime reached, terminating notification stream'); + break; + } counter++; try { await sendNotification({ method: 'notifications/message', params: { level: 'info', data: `Periodic notification #${counter}`, }, }); } catch (error) { console.error('Error sending notification:', error); } await sleep(interval); }core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts (1)
34-45: Duplicated code pattern in metadata buildersThe buildPrompt and buildTool methods follow identical patterns. Consider refactoring to a generic method to reduce duplication.
+ private buildMetadata<T>( + getMethodNames: (clazz: EggProtoImplClass) => string[], + createBuilder: (clazz: EggProtoImplClass, methodName: string) => { build(): T | null } + ): T[] { + const methodNames = getMethodNames(this.clazz); + const methods: T[] = []; + for (const methodName of methodNames) { + const builder = createBuilder(this.clazz, methodName); + const methodMeta = builder.build(); + if (methodMeta) { + methods.push(methodMeta); + } + } + return methods; + } + + private buildResource(): MCPResourceMeta[] { + return this.buildMetadata<MCPResourceMeta>( + MCPInfoUtil.getMCPResource, + (clazz, methodName) => new MCPControllerResourceMetaBuilder(clazz, methodName) + ); + } + + private buildPrompt(): MCPPromptMeta[] { + return this.buildMetadata<MCPPromptMeta>( + MCPInfoUtil.getMCPPrompt, + (clazz, methodName) => new MCPControllerPromptMetaBuilder(clazz, methodName) + ); + } + + private buildTool(): MCPToolMeta[] { + return this.buildMetadata<MCPToolMeta>( + MCPInfoUtil.getMCPTool, + (clazz, methodName) => new MCPControllerToolMetaBuilder(clazz, methodName) + ); + }Also applies to: 47-58
core/types/controller-decorator/MCPController.ts (1)
11-14: Consider improving type safety for the preProxy hook parameters.The current parameter type for
proxyReqandproxyRespusesParameters<typeof http.createServer>which references the request and response handler parameters. While this works, it might be more explicit and type-safe to directly usehttp.IncomingMessageandhttp.ServerResponsetypes.- preProxy?: (ctx: Context, proxyReq: Parameters<typeof http.createServer>['0'], proxyResp: Parameters<typeof http.createServer>['1']) => Promise<void> + preProxy?: (ctx: Context, proxyReq: http.IncomingMessage, proxyResp: http.ServerResponse) => Promise<void>plugin/mcp-proxy/test/proxy.test.ts (2)
25-53: Consider error handling for the notification collection.The function properly collects notifications, but it doesn't handle potential errors in the result content parsing. Consider adding try/catch blocks or validation to ensure robust error handling.
result.content.forEach(item => { + try { if (item.type === 'text') { notifications.push({ text: item.text, }); } else { + if (!item.data) { + throw new Error('Item data is undefined'); + } notifications.push({ text: item.data!.toString(), }); } + } catch (error) { + console.error('Error processing notification item:', error); + } });
93-124: Add timeout mechanism for notification collection.The test waits 5 seconds using
setTimeoutregardless of how many notifications were actually received. Consider adding a more robust mechanism to wait until all expected notifications are received or until a timeout occurs.await sseClient.connect(sseTransport); const tools = await listTools(sseClient); const notificationResp = await startNotificationTool(sseClient); - await new Promise(resolve => setTimeout(resolve, 5000)); + // Wait for all notifications or timeout after 10 seconds + await new Promise((resolve, reject) => { + const maxWaitTime = 10000; // 10 seconds + const expectedNotifications = 5; + + const checkInterval = setInterval(() => { + if (sseNotifications.length >= expectedNotifications) { + clearInterval(checkInterval); + clearTimeout(timeout); + resolve(undefined); + } + }, 100); + + const timeout = setTimeout(() => { + clearInterval(checkInterval); + if (sseNotifications.length < expectedNotifications) { + reject(new Error(`Timed out waiting for notifications. Expected ${expectedNotifications}, received ${sseNotifications.length}`)); + } else { + resolve(undefined); + } + }, maxWaitTime); + }); await sseTransport.close();plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (1)
60-82: Add validation for resource URI parameters.The
carmethod accepts a URI but doesn't validate that the required parameters are present. Consider adding validation to ensure the URI contains the expected parameters before processing.async car(uri: URL): Promise<MCPResourceResponse> { + // Validate URI parameters + const params = new URLSearchParams(uri.search); + const name = uri.pathname.split('/').pop(); + if (!name) { + throw new Error('Missing required parameter: name'); + } + return { contents: [{ uri: uri.toString(), text: 'MOCK TEXT', }], }; }core/controller-decorator/test/fixtures/MCPController.ts (2)
5-5: Prefer named import for Zod to avoid relying onesModuleInterop
zodhas no default export; importing the default works only whenesModuleInterop/allowSyntheticDefaultImportsis enabled.
Use a named import to keep the fixture compile-time agnostic of ts-config settings.-import z from 'zod'; +import { z } from 'zod';
56-57: Inconsistent URI schemehitu://– likely a typoAll other fixtures/tests use the
mcp://scheme. A mismatched scheme will make this controller invisible when clients filter by protocol.
Please verify and align.plugin/controller/test/mcp/mcp.test.ts (1)
129-131: 5 ssetTimeoutslows test suite – consider event-driven awaitInstead of a fixed sleep, await the expected number of notifications via a Promise that resolves when the array length meets the target. This drops test time from 5 s to sub-second and makes failures deterministic.
Also applies to: 220-222
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (3)
163-169: Unnecessaryconst self = this– arrow functions already capture lexicalthisRemove the alias and reference
thisdirectly to silence the lint warning and improve clarity.- const self = this; const transport = new SSEServerTransport('/message', this.ctx.res); // register handler and client demo - this.app.mcpProxy.setProxyHandler('SSE', async (req, res) => { - return await self.ssePostHandler(req, res); - }); + this.app.mcpProxy.setProxyHandler('SSE', async (req, res) => { + return await this.ssePostHandler(req, res); + });🧰 Tools
🪛 Biome (1.9.4)
[error] 163-163: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
246-254: Samethisalias pattern repeated – apply the earlier fix- const self = this; const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID(), eventStore, onsessioninitialized: async sessionId => { transports[sessionId] = transport; - this.app.mcpProxy.setProxyHandler('STREAM', async (req, res) => { - return await self.streamPostHandler(req, res); + this.app.mcpProxy.setProxyHandler('STREAM', async (req, res) => { + return await this.streamPostHandler(req, res); }); await this.app.mcpProxy.registerClient(sessionId, process.pid); }, });🧰 Tools
🪛 Biome (1.9.4)
[error] 246-246: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
58-84: Potential unbounded memory growth inInMemoryEventStore
eventsis an ever-growingMapwithout TTL or size cap. Long-lived servers can exhaust memory if clients stream large volumes.
Consider purging old events per stream or moving to a bounded queue.core/controller-decorator/src/util/MCPInfoUtil.ts (1)
1-9: Utility can be simplified – static-only class is unnecessary
MCPInfoUtilcontains only static methods and no private state. Replacing it with a file-level function collection (or a namespace export) removes thenoStaticOnlyClasssmell, produces leaner JS and avoids accidental instantiation.plugin/mcp-proxy/index.ts (2)
55-69: Typo in proxy-action constant may bite integrations
'MCP_SEE_PROXY'(double “E”) is used for SSE pathways.
Internally the typo is consistent, but any external code that expects the more obvious'MCP_SSE_PROXY'will fail to match.Renaming now (and keeping the misspelt value as a deprecated alias) will avoid future confusion.
-type ProxyAction = 'MCP_STDIO_PROXY' | 'MCP_SEE_PROXY' | 'MCP_STREAM_PROXY'; +type ProxyAction = 'MCP_STDIO_PROXY' | 'MCP_SSE_PROXY' | 'MCP_STREAM_PROXY'; ... - action = 'MCP_SEE_PROXY'; + action = 'MCP_SSE_PROXY';
127-131: Avoiddelete headers['transfer-encoding']– reassign for better perfThe
deleteoperator de-optimises hidden classes in V8.
Simply overwrite the key instead:- delete headers['transfer-encoding']; + headers['transfer-encoding'] = undefined as unknown as string;Also applies to: 167-171
🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
70-73:const self = thisis redundant in arrow functionsAll nested callbacks here are arrow functions which already capture
this.
Removing the extra alias improves readability and keeps “one source of truth”.- const allRouterFunc = this.router.all; - const self = this; + const allRouterFunc = this.router.all;Apply the same simplification to later occurrences (
router.get,router.post, etc.).🧰 Tools
🪛 Biome (1.9.4)
[error] 72-72: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
108-144:setProxyHandler('STREAM', …)is registered on every sessionEach new stream session re-assigns the global handler, but the handler itself already dispatches by
sessionId.
Register once during server initialisation to avoid unnecessary overwrites and potential race conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
.eslintrc(1 hunks)core/controller-decorator/index.ts(2 hunks)core/controller-decorator/package.json(1 hunks)core/controller-decorator/src/decorator/mcp/MCPController.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPPrompt.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPResource.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPTool.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerResourceMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerToolMetaBuilder.ts(1 hunks)core/controller-decorator/src/model/MCPControllerMeta.ts(1 hunks)core/controller-decorator/src/model/MCPPromptMeta.ts(1 hunks)core/controller-decorator/src/model/MCPResourceMeta.ts(1 hunks)core/controller-decorator/src/model/MCPToolMeta.ts(1 hunks)core/controller-decorator/src/model/index.ts(1 hunks)core/controller-decorator/src/util/MCPInfoUtil.ts(1 hunks)core/controller-decorator/test/MCPMeta.test.ts(1 hunks)core/controller-decorator/test/fixtures/MCPController.ts(1 hunks)core/types/controller-decorator/MCPController.ts(1 hunks)core/types/controller-decorator/MCPPromptParams.ts(1 hunks)core/types/controller-decorator/MCPResourceParams.ts(1 hunks)core/types/controller-decorator/MCPToolParams.ts(1 hunks)core/types/controller-decorator/MetadataKey.ts(1 hunks)core/types/controller-decorator/index.ts(1 hunks)core/types/controller-decorator/model/types.ts(1 hunks)core/types/package.json(1 hunks)plugin/controller/agent.ts(1 hunks)plugin/controller/app.ts(4 hunks)plugin/controller/app/middleware/mcp_body_middleware.ts(1 hunks)plugin/controller/config/config.default.ts(1 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/controller/package.json(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/config/config.default.js(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/package.json(1 hunks)plugin/controller/test/mcp/mcp.test.ts(1 hunks)plugin/controller/tsconfig.json(1 hunks)plugin/controller/typings/index.d.ts(1 hunks)plugin/mcp-proxy/CHANGELOG.md(1 hunks)plugin/mcp-proxy/README.md(1 hunks)plugin/mcp-proxy/app/extend/agent.ts(1 hunks)plugin/mcp-proxy/app/extend/application.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)plugin/mcp-proxy/lib/MCPProxyDataClient.ts(1 hunks)plugin/mcp-proxy/package.json(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/router.ts(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/config.default.js(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/plugin.js(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/package.json(1 hunks)plugin/mcp-proxy/test/proxy.test.ts(1 hunks)plugin/mcp-proxy/tsconfig.json(1 hunks)plugin/mcp-proxy/tsconfig.pub.json(1 hunks)plugin/mcp-proxy/typings/index.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (17)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/config.default.js (2)
plugin/controller/config/config.default.ts (1)
config(3-14)plugin/controller/test/fixtures/apps/mcp-app/config/config.default.js (1)
config(4-11)
plugin/controller/app/middleware/mcp_body_middleware.ts (1)
core/types/controller-decorator/model/types.ts (2)
EggContext(3-3)Next(4-4)
plugin/controller/test/fixtures/apps/mcp-app/config/config.default.js (2)
plugin/controller/config/config.default.ts (1)
config(3-14)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/config.default.js (1)
config(4-14)
plugin/mcp-proxy/app/extend/application.ts (1)
plugin/mcp-proxy/index.ts (1)
MCPProxyApiClient(30-239)
core/controller-decorator/src/decorator/mcp/MCPResource.ts (4)
core/types/controller-decorator/MCPResourceParams.ts (1)
MCPResourceParams(19-19)core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)core/controller-decorator/src/util/MethodInfoUtil.ts (1)
MethodInfoUtil(24-116)core/controller-decorator/src/util/MCPInfoUtil.ts (1)
MCPInfoUtil(9-96)
plugin/controller/app.ts (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
MCPControllerRegister(33-451)
plugin/controller/typings/index.d.ts (1)
plugin/mcp-proxy/typings/index.d.ts (1)
MCPProxyApp(6-8)
plugin/mcp-proxy/typings/index.d.ts (2)
plugin/mcp-proxy/index.ts (1)
MCPProxyApiClient(30-239)plugin/controller/typings/index.d.ts (1)
Application(15-16)
core/controller-decorator/src/decorator/mcp/MCPPrompt.ts (4)
core/types/controller-decorator/MCPPromptParams.ts (1)
MCPPromptParams(10-14)core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)core/controller-decorator/src/util/MethodInfoUtil.ts (1)
MethodInfoUtil(24-116)core/controller-decorator/src/util/MCPInfoUtil.ts (1)
MCPInfoUtil(9-96)
plugin/controller/config/config.default.ts (2)
plugin/controller/test/fixtures/apps/mcp-app/config/config.default.js (1)
config(4-11)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/config.default.js (1)
config(4-14)
core/controller-decorator/test/MCPMeta.test.ts (3)
core/controller-decorator/src/builder/ControllerMetaBuilderFactory.ts (1)
ControllerMetaBuilderFactory(11-50)core/controller-decorator/src/model/MCPControllerMeta.ts (1)
MCPControllerMeta(7-70)core/controller-decorator/test/fixtures/MCPController.ts (2)
ToolType(12-16)PromptType(8-10)
core/controller-decorator/src/model/MCPToolMeta.ts (1)
core/types/controller-decorator/model/types.ts (1)
MiddlewareFunc(5-5)
core/controller-decorator/src/model/MCPResourceMeta.ts (1)
core/types/controller-decorator/model/types.ts (1)
MiddlewareFunc(5-5)
core/controller-decorator/src/model/MCPControllerMeta.ts (5)
core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggPrototypeName(6-6)core/types/controller-decorator/model/types.ts (1)
MiddlewareFunc(5-5)core/controller-decorator/src/model/MCPToolMeta.ts (1)
MCPToolMeta(5-31)core/controller-decorator/src/model/MCPResourceMeta.ts (1)
MCPResourceMeta(5-36)core/controller-decorator/src/model/MCPPromptMeta.ts (1)
MCPPromptMeta(4-30)
core/controller-decorator/src/model/MCPPromptMeta.ts (1)
core/types/controller-decorator/model/types.ts (1)
MiddlewareFunc(5-5)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (9)
core/controller-decorator/test/fixtures/MCPController.ts (3)
PromptType(8-10)ToolType(12-16)MCPController(18-71)core/controller-decorator/src/decorator/mcp/MCPController.ts (1)
MCPController(6-32)plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts (1)
MCPController(21-60)core/controller-decorator/src/decorator/mcp/MCPPrompt.ts (1)
MCPPrompt(9-32)core/types/controller-decorator/MCPPromptParams.ts (2)
PromptArgs(5-5)MCPPromptResponse(8-8)core/controller-decorator/src/decorator/mcp/MCPTool.ts (1)
MCPTool(9-32)core/types/controller-decorator/MCPToolParams.ts (2)
ToolArgs(5-5)MCPToolResponse(8-8)core/controller-decorator/src/decorator/mcp/MCPResource.ts (1)
MCPResource(9-32)core/types/controller-decorator/MCPResourceParams.ts (1)
MCPResourceResponse(21-21)
core/controller-decorator/test/fixtures/MCPController.ts (8)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (3)
PromptType(14-16)ToolType(18-22)MCPController(25-83)core/controller-decorator/src/decorator/mcp/MCPController.ts (1)
MCPController(6-32)core/controller-decorator/src/decorator/mcp/MCPPrompt.ts (1)
MCPPrompt(9-32)core/types/controller-decorator/MCPPromptParams.ts (2)
PromptArgs(5-5)MCPPromptResponse(8-8)core/controller-decorator/src/decorator/mcp/MCPTool.ts (1)
MCPTool(9-32)core/types/controller-decorator/MCPToolParams.ts (2)
ToolArgs(5-5)MCPToolResponse(8-8)core/controller-decorator/src/decorator/mcp/MCPResource.ts (1)
MCPResource(9-32)core/types/controller-decorator/MCPResourceParams.ts (1)
MCPResourceResponse(21-21)
🪛 Biome (1.9.4)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/config.default.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
plugin/controller/test/fixtures/apps/mcp-app/config/config.default.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
core/controller-decorator/src/decorator/mcp/MCPController.ts
[error] 7-7: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/plugin.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
core/controller-decorator/test/MCPMeta.test.ts
[error] 8-8: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
plugin/controller/test/mcp/mcp.test.ts
[error] 88-90: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts
[error] 163-163: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 246-246: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/mcp-proxy/index.ts
[error] 130-130: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 170-170: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
core/controller-decorator/src/util/MCPInfoUtil.ts
[error] 9-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
plugin/mcp-proxy/test/proxy.test.ts
[error] 89-91: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 72-72: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 204-204: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 278-278: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 333-333: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 368-368: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 391-391: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
🪛 markdownlint-cli2 (0.17.2)
plugin/mcp-proxy/CHANGELOG.md
356-356: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
653-653: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
664-664: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
819-819: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
830-830: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
849-849: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
924-924: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (55)
core/types/package.json (1)
40-40: Approve addition of MCP SDK devDependency
Adding@modelcontextprotocol/sdk@^1.10.0todevDependenciesaligns with the new MCP type definitions.plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/package.json (1)
1-3: Approve minimal package.json for MCP proxy fixture
The fixture’spackage.jsoncorrectly defines the minimal manifest for the test app.core/controller-decorator/package.json (1)
46-46: Approve addition of MCP SDK dependency
Including@modelcontextprotocol/sdk@^1.10.0underdependenciessupports the new MCP decorators and metadata builders.core/types/controller-decorator/model/types.ts (1)
9-11: Introduce MCP ControllerType enum
The newControllerType.MCPentry integrates MCP into the controller ecosystem. Ensure any switch statements or mappings consumingControllerTypeare updated to handle this new type.plugin/controller/test/fixtures/apps/mcp-app/package.json (1)
1-3: Approve minimal package.json for MCP controller fixture
Defines the base manifest for thecontroller-apptest fixture appropriately.plugin/controller/package.json (1)
56-59:Details
❓ Verification inconclusive
Review new MCP dependencies and version alignment.
Dependencies
@eggjs/mcp-proxy@^3.53.0,@modelcontextprotocol/sdk@^1.10.0,content-type@^1.0.5, andraw-body@^2.5.2have been added. Ensure these versions align with the rest of the ecosystem and verify no critical vulnerabilities are introduced.Please run:
to confirm there are no high-severity advisories for these new packages.
🏁 Script executed:
npm audit --json | jq '.advisories | keys'Length of output: 443
🏁 Script executed:
#!/bin/bash # Generate a lockfile and re-run audit to catch any high/critical advisories npm i --package-lock-only npm audit --json | jq '.advisories | to_entries[] | select(.value.severity == "high" or .value.severity == "critical")'Length of output: 732
Please verify new MCP dependencies and run
npm auditThe added packages (
@eggjs/mcp-proxy@^3.53.0,@modelcontextprotocol/sdk@^1.10.0,content-type@^1.0.5,raw-body@^2.5.2) should be checked for high- or critical-severity vulnerabilities. Since there’s no lockfile, please:
- Commit a
package-lock.json(e.g.npm i --package-lock-only).- Run
npm audit --json | jq '.advisories | to_entries[] | select(.value.severity=="high" or .value.severity=="critical")'.- Confirm that none of these new dependencies introduce any high- or critical-severity advisories.
plugin/mcp-proxy/tsconfig.pub.json (1)
1-10: TypeScript publish configuration is correct.Extending the workspace
../../tsconfig.jsonand excludingnode_modulesandtestis appropriate for the published package.plugin/mcp-proxy/tsconfig.json (1)
1-10: TypeScript development configuration is valid.Extending the root
tsconfig.jsonand excludingnode_modulesandtestdirectories aligns with best practices for plugin development.plugin/controller/tsconfig.json (1)
6-8: LGTM: Enables compilation of MCP test filesRemoving "test" from the exclude array allows TypeScript to compile the newly added MCP test files. This is necessary for the new MCP controller tests to be properly built and executed.
plugin/controller/typings/index.d.ts (2)
3-3: LGTM: Required import for MCP proxy typesAdding the import for '@eggjs/mcp-proxy' makes the MCPProxyApp interface available for use in this file.
9-9: LGTM: Correctly extends TEggControllerApp with MCP proxy capabilitiesThis change ensures that the TEggControllerApp interface includes the mcpProxy property, allowing type-safe access to MCP proxy functionality in controller applications.
plugin/controller/test/fixtures/apps/mcp-app/config/config.default.js (1)
4-11: LGTM: Appropriate test configuration for MCP testingThe configuration disables CSRF protection, which is a common practice for test fixtures to simplify API testing. This aligns with similar configurations in other MCP-related files.
plugin/controller/agent.ts (1)
3-9: LGTM: Well-structured agent hook classThe AppHook class is properly initialized with the agent instance and follows the standard pattern for Egg.js lifecycle hooks.
plugin/controller/config/config.default.ts (1)
1-14: The CSRF protection configuration looks goodThis configuration correctly establishes a regex pattern to bypass CSRF protection for MCP-related routes. This is an important security configuration that allows MCP endpoints to function properly while maintaining CSRF protection for other routes.
plugin/mcp-proxy/app/extend/agent.ts (1)
1-17: Implementation follows Egg.js extension pattern correctlyThe implementation of the
mcpProxygetter properly follows the Egg.js extension pattern using a Symbol to avoid naming collisions and lazy-loading the client only when needed. The singleton pattern ensures only one instance of the MCPProxyApiClient per agent.plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/config.default.js (1)
3-16: Test configuration is properly structuredThe test configuration correctly disables both CSRF protection and the body parser, which is necessary for testing MCP proxy functionality without middleware interference.
plugin/mcp-proxy/app/extend/application.ts (1)
1-16: Implementation correctly extends Application with mcpProxyThis implementation correctly follows the Egg.js pattern for extending the Application object with a new property. The lazy-loading pattern and use of Symbol ensure efficiency and avoid naming collisions.
plugin/controller/app/middleware/mcp_body_middleware.ts (1)
4-18: Middleware implementation is correctThe middleware correctly identifies MCP-specific paths and disables the body parser for those routes, which is necessary for proper handling of MCP requests. The implementation uses path-to-regexp for accurate path matching.
core/controller-decorator/src/model/index.ts (1)
6-9: Exports look good.The export statements for the new MCP metadata classes follow the existing pattern in the file. This provides a clean public API for the MCP-related metadata models.
plugin/controller/app.ts (3)
14-14: Import of MCPControllerRegister looks good.The import statement correctly references the MCP controller register implementation.
45-45: MCP controller registration looks good.The code properly registers the MCPControllerRegister with the controller register factory for the MCP controller type.
66-67: Middleware addition looks good.Adding the mcpBodyMiddleware at the beginning of the middleware chain ensures that MCP request bodies won't be consumed by other middleware.
core/types/controller-decorator/index.ts (1)
9-12: MCP module exports look goodThe new exports for MCP-related modules follow the existing pattern and naming conventions of the file, properly extending the controller decorator types to support the new Model Context Protocol feature.
core/controller-decorator/src/impl/mcp/MCPControllerToolMetaBuilder.ts (1)
1-35: Well-structured metadata builder implementationThe
MCPControllerToolMetaBuilderfollows a clean builder pattern and properly collects all necessary metadata for MCP tool methods. The implementation is well-typed and handles edge cases appropriately.core/controller-decorator/src/decorator/mcp/MCPPrompt.ts (2)
9-32: LGTM! The decorator implementation follows best practices.The decorator handles optional parameters correctly, properly separates concerns between setting controller type and MCP-specific information, and correctly types the function parameters.
9-32:Details
❌ Incorrect review comment
Check for potential Symbol property keys.
The code casts
propertyKeyto string on line 15, but if the property key is a Symbol, this might lead to unexpected behavior when using it as a map key.Consider adding a check for Symbol property keys or ensuring that the expected behavior is documented:
- const methodName = propertyKey as string; + // Handle both string and Symbol property keys + const methodName = propertyKey.toString();Run the following to check if any Symbol property keys are used in the codebase:
🏁 Script executed:
#!/bin/bash # Search for Symbol property key usage in decorators rg "Symbol\(" --type=tsLength of output: 2015
Ignore Symbol key handling suggestion
The
propertyKeyin decorators is always a string in our codebase—no symbol-named methods are ever decorated with@MCPPrompt. A search forSymbol(only reveals symbol definitions, not symbol-based method names. Casting withas stringis sufficient and equivalent totoString()for all current use cases. No changes are needed.Likely an incorrect or invalid review comment.
plugin/mcp-proxy/typings/index.d.ts (1)
1-12: LGTM! The type declaration follows TypeScript best practices.The declaration file properly extends the egg module with MCP proxy functionality using TypeScript's declaration merging, making the
mcpProxyproperty type-safe and accessible on the application instance.core/controller-decorator/src/decorator/mcp/MCPController.ts (1)
6-32: LGTM! The decorator implementation is well-structured.Overall, the decorator handles optional parameters gracefully, properly composes multiple decorations, and extracts metadata automatically to simplify usage.
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
core/controller-decorator/src/model/MCPToolMeta.ts (1)
5-31: LGTM! The metadata class is well-structured.The class uses proper TypeScript typing, readonly properties for immutability, and a structured options object for initialization. The implementation correctly handles optional parameters and provides a clean interface for tool metadata.
plugin/mcp-proxy/package.json (1)
1-69: Overall package structure looks goodThe package definition is well-structured with appropriate dependencies for an MCP proxy implementation. The inclusion of cluster-client, content-type handling, and raw-body parsing aligns with the needs of a proxy service handling Model Context Protocol.
core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts (1)
1-35: Overall implementation is clean and follows the builder pattern wellThe builder implementation follows the established patterns in the codebase, with a clear separation of concerns and proper metadata collection. The code is well-structured and maintainable.
core/controller-decorator/test/MCPMeta.test.ts (1)
1-25: The test coverage is comprehensiveThe test covers all essential aspects of the MCP metadata building process, including the controller properties, tools, resources, and prompts. It verifies that the metadata is correctly built and that the properties have the expected types and values.
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/plugin.js (1)
1-22: Configuration looks correct for test purposesThe plugin configuration properly enables the required plugins for testing the MCP proxy functionality. Disabling the watcher is also appropriate for test environments.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
core/controller-decorator/src/model/MCPResourceMeta.ts (1)
1-36: Well-structured implementation for MCP resource metadata handling.The
MCPResourceMetaclass provides a clean encapsulation of resource metadata for the Model Context Protocol system. It properly handles various types of metadata, including ACL flags, URI paths, templates, and middlewares.A few implementation details to note:
- The use of
readonlyfor class properties enforces immutability- Boolean normalization using
!!on line 26 ensures proper type conversion- Conditional template instantiation provides flexibility in usage
plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts (2)
10-19: Zod schema definition looks good!The Zod schema for
NotificationTypeis well-defined with appropriate descriptions and default values.
51-58: Good response formatThe response format follows the MCP protocol requirements with the proper content structure.
core/controller-decorator/index.ts (3)
2-2: Good addition of MCPControllerMetaBuilder importThis import correctly brings in the MCP controller metadata builder implementation.
13-17: Well-structured exports for MCP decoratorsThe MCP decorator exports are logically organized alongside the existing HTTP decorator exports.
19-19: Proper export of MCPInfoUtil utilityThe MCPInfoUtil is correctly exported as a default import, consistent with other utility exports.
core/controller-decorator/src/model/MCPPromptMeta.ts (3)
1-2: Good type importsThe imports for MiddlewareFunc and McpServer types are appropriate and will ensure proper type safety.
4-12: Well-designed metadata class propertiesThe MCPPromptMeta class has a good set of properties for MCP prompts with appropriate readonly modifiers. The use of Parameters<McpServer['prompt']>['2'] for schema type is a clever way to maintain type compatibility with the MCP server API.
13-29: Constructor implementation looks goodThe constructor properly initializes all properties and handles optional fields. The conversion of
needAclto boolean using!!ensures type consistency.core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts (3)
21-32: Good implementation of resource metadata collectionThe buildResource method correctly retrieves resource method names and builds metadata for each one.
60-80: Thorough metadata collection in build methodThe build method properly validates the controller type, collects all required metadata, and constructs the final MCPControllerMeta. The use of assert for type validation is a good practice.
82-87: Factory method and registrationThe static create method and registration with ControllerMetaBuilderFactory follow the established pattern in the codebase.
core/types/controller-decorator/MCPPromptParams.ts (1)
1-14: Well-structured type definitionsThis file provides a solid foundation for type-safe handling of MCP prompts. The types are clearly defined and properly extract parameter types from the MCP server's
promptmethod.core/types/controller-decorator/MCPToolParams.ts (1)
1-15: Type definitions follow consistent patternThe tool parameter types are well-defined and follow the same pattern as the prompt parameters, which maintains consistency across the MCP type system.
core/types/controller-decorator/MCPResourceParams.ts (1)
1-21: Appropriate dual-approach to resource identificationThe type definitions effectively handle both URI-based and template-based resource identification patterns, providing flexibility in how resources can be defined and accessed.
core/controller-decorator/src/model/MCPControllerMeta.ts (1)
7-70: Overall class structure is well-designedThe
MCPControllerMetaclass properly implements theControllerMetadatainterface and provides good utility methods for handling middleware and ACL requirements. The class effectively organizes metadata for MCP tools, resources, and prompts.core/types/controller-decorator/MetadataKey.ts (1)
27-34: Well-structured MCP metadata keys.The implementation of MCP-related metadata keys follows the established pattern in the codebase. The naming convention is consistent with existing metadata keys, which makes the code more maintainable and readable.
core/types/controller-decorator/MCPController.ts (1)
4-9: LGTM: Clear interface definition for MCP controller parameters.The
MCPControllerParamsinterface provides a well-defined structure for configuring MCP controllers with optional parameters.plugin/mcp-proxy/test/proxy.test.ts (1)
9-24: LGTM: Well-implemented helper functions for MCP testing.These helper functions encapsulate common operations nicely, improving code reusability and readability in the tests.
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (3)
14-16: LGTM: Well-defined schema types using Zod.The schema definitions using Zod are clear and include appropriate descriptions for better documentation.
Also applies to: 18-22
25-43: LGTM: Well-implemented MCPPrompt method.The
foomethod is properly decorated with@MCPPromptand uses the correct schema and return type. The implementation is straightforward and follows the same pattern as in the test fixtures.
45-57: LGTM: Well-implemented MCPTool method.The
barmethod is properly decorated with@MCPTooland uses the correct schema and return type. The implementation returns an appropriate response structure.
core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js (1)
1-1: Remove redundant 'use strict' directive
Node.js modules run in strict mode by default—this directive can be omitted to reduce noise.🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugin/controller/agent.ts(1 hunks)plugin/controller/app.ts(4 hunks)plugin/controller/package.json(2 hunks)plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin/controller/agent.ts
- plugin/controller/package.json
- plugin/controller/app.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (20)
🔇 Additional comments (5)
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js (5)
3-6: Verify tracer plugin configuration
Theegg-tracerexport is correctly defined and enabled. Ensure the version ofegg-tracerinpackage.jsonaligns with your test fixture’s requirements.
8-11: Verify tegg plugin configuration
The@eggjs/tegg-pluginexport follows conventions and is enabled. Confirm it's listed as a dependency and matches your test app’s version constraints.
13-16: Verify tegg-config plugin configuration
The@eggjs/tegg-configplugin is properly exported and enabled. Double-check that any required config files are available in your fixture.
18-21: Verify aopModule plugin configuration
The@eggjs/tegg-aop-pluginis correctly exported here. There’s no duplicate export in this file, so the previous duplication warning does not apply.
23-26: Verify mcpProxy plugin configuration
The new@eggjs/mcp-proxyplugin is enabled as intended for MCP tests. Ensure this matches the MCP proxy package version your pull request introduces.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
plugin/mcp-proxy/test/proxy.test.ts (2)
95-160: DRY: Extract common test logic to a shared function.The "streamable should work" test duplicates much of the logic from the "sse should work" test. Consider refactoring to extract the common test logic into a shared function to improve maintainability.
+ async function testTransport(transportType, createTransport, getBaseUrl) { + const client = new Client({ + name: `${transportType}-demo-client`, + version: '1.0.0', + }); + const baseUrl = await getBaseUrl(); + const transport = createTransport(new URL(baseUrl)); + const notifications = []; + client.setNotificationHandler(LoggingMessageNotificationSchema, notification => { + notifications.push({ level: notification.params.level, data: notification.params.data as string }); + }); + await client.connect(transport); + const tools = await listTools(client); + const notificationResp = await startNotificationTool(client); + await new Promise(resolve => setTimeout(resolve, 5000)); + + if (transportType === 'streamable') { + await transport.terminateSession(); + await client.close(); + } else { + await transport.close(); + } + + assert.deepEqual(tools, [ + { + name: 'start-notification-stream', + description: 'Starts sending periodic notifications for testing resumability', + }, + ]); + assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); + assert.deepEqual(notifications, [ + { level: 'info', data: 'Periodic notification #1' }, + { level: 'info', data: 'Periodic notification #2' }, + { level: 'info', data: 'Periodic notification #3' }, + { level: 'info', data: 'Periodic notification #4' }, + { level: 'info', data: 'Periodic notification #5' }, + ]); + } it('sse should work', async () => { - const sseClient = new Client({ - name: 'sse-demo-client', - version: '1.0.0', - }); - const baseUrl = await app.httpRequest() - .get('/init').url; - const sseTransport = new SSEClientTransport(new URL(baseUrl)); - const sseNotifications: { level: string, data: string }[] = []; - sseClient.setNotificationHandler(LoggingMessageNotificationSchema, notification => { - sseNotifications.push({ level: notification.params.level, data: notification.params.data as string }); - }); - await sseClient.connect(sseTransport); - const tools = await listTools(sseClient); - const notificationResp = await startNotificationTool(sseClient); - await new Promise(resolve => setTimeout(resolve, 5000)); - await sseTransport.close(); - assert.deepEqual(tools, [ - { - name: 'start-notification-stream', - description: 'Starts sending periodic notifications for testing resumability', - }, - ]); - assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); - assert.deepEqual(sseNotifications, [ - { level: 'info', data: 'Periodic notification #1' }, - { level: 'info', data: 'Periodic notification #2' }, - { level: 'info', data: 'Periodic notification #3' }, - { level: 'info', data: 'Periodic notification #4' }, - { level: 'info', data: 'Periodic notification #5' }, - ]); + await testTransport( + 'sse', + url => new SSEClientTransport(url), + async () => await app.httpRequest().get('/init').url + ); }); it('streamable should work', async () => { - const streamableClient = new Client({ - name: 'streamable-demo-client', - version: '1.0.0', - }); - const baseUrl = await app.httpRequest() - .post('/stream').url; - const streamableTransport = new StreamableHTTPClientTransport(new URL(baseUrl)); - const streamableNotifications: { level: string, data: string }[] = []; - streamableClient.setNotificationHandler(LoggingMessageNotificationSchema, notification => { - streamableNotifications.push({ level: notification.params.level, data: notification.params.data as string }); - }); - await streamableClient.connect(streamableTransport); - const tools = await listTools(streamableClient); - const notificationResp = await startNotificationTool(streamableClient); - await new Promise(resolve => setTimeout(resolve, 5000)); - await streamableTransport.terminateSession(); - await streamableClient.close(); - assert.deepEqual(tools, [ - { - name: 'start-notification-stream', - description: 'Starts sending periodic notifications for testing resumability', - }, - ]); - assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); - assert.deepEqual(streamableNotifications, [ - { level: 'info', data: 'Periodic notification #1' }, - { level: 'info', data: 'Periodic notification #2' }, - { level: 'info', data: 'Periodic notification #3' }, - { level: 'info', data: 'Periodic notification #4' }, - { level: 'info', data: 'Periodic notification #5' }, - ]); + await testTransport( + 'streamable', + url => new StreamableHTTPClientTransport(url), + async () => await app.httpRequest().post('/stream').url + ); });
91-93:⚠️ Potential issueRemove duplicate after hook.
There are two identical
afterhooks that callapp.close(). This was flagged by the static analysis tool as a duplicate hook, which can lead to confusion and potential issues.after(async () => { await app.close(); }); afterEach(() => { // mm.restore(); }); before(async () => { mm(process.env, 'EGG_TYPESCRIPT', true); mm(process, 'cwd', () => { return path.join(__dirname, '..'); }); app = mm.cluster({ baseDir: path.join(__dirname, 'fixtures/apps/mcp-proxy'), framework: path.dirname(require.resolve('egg')), // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore workers: 4, sticky: false, opt: { env: { ...process.env, NODE_OPTIONS: '--require ts-node/register tsconfig-paths/register', }, }, }); await app.ready(); }); - after(() => { - return app.close(); - });🧰 Tools
🪛 Biome (1.9.4)
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
plugin/controller/test/mcp/mcp.test.ts (1)
91-93:⚠️ Potential issueDuplicate
after()hooks – causes Biome lint errorTwo identical
afterhooks exist inside the samedescribe, triggeringnoDuplicateTestHooksand risking double shutdown.Keep one and remove the other:
- after(() => { - return app.close(); - });🧰 Tools
🪛 Biome (1.9.4)
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🧹 Nitpick comments (6)
plugin/mcp-proxy/test/proxy.test.ts (2)
66-66: Clean up commented code.The commented
mm.restore()should either be removed entirely or uncommented if needed. Commented-out code can lead to confusion about its purpose and necessity.afterEach(() => { - // mm.restore(); });
110-110: Extract hardcoded timeout to a constant.The hardcoded timeout value of 5000ms appears in multiple places. Extract it to a named constant at the top of the file to improve maintainability and ensure consistency.
+ const NOTIFICATION_WAIT_TIMEOUT_MS = 5000; ... - await new Promise(resolve => setTimeout(resolve, 5000)); + await new Promise(resolve => setTimeout(resolve, NOTIFICATION_WAIT_TIMEOUT_MS));Also applies to: 143-143
plugin/controller/test/mcp/mcp.test.ts (4)
66-66: Clean up commented code.The commented
mm.restore()should either be removed entirely or uncommented if needed. Commented-out code can lead to confusion about its purpose and necessity.afterEach(() => { - // mm.restore(); });
132-132: Extract hardcoded timeout to a constant.The hardcoded timeout value of 5000ms appears in multiple places. Extract it to a named constant at the top of the file to improve maintainability and ensure consistency.
+ const NOTIFICATION_WAIT_TIMEOUT_MS = 5000; ... - await new Promise(resolve => setTimeout(resolve, 5000)); + await new Promise(resolve => setTimeout(resolve, NOTIFICATION_WAIT_TIMEOUT_MS));Also applies to: 223-223
1-54: Extract common helper functions to a shared utility file.The helper functions
listToolsandstartNotificationToolare duplicated between this file andplugin/mcp-proxy/test/proxy.test.ts. Consider extracting these to a shared test utility file to improve maintainability and reduce duplication.// Create a new file: test/utils/mcp-test-utils.ts + import { Client } from '@modelcontextprotocol/sdk/client/index.js'; + import { CallToolRequest, CallToolResultSchema, ListToolsRequest, ListToolsResultSchema } from '@modelcontextprotocol/sdk/types.js'; + + export async function listTools(client: Client) { + const toolsRequest: ListToolsRequest = { + method: 'tools/list', + params: {}, + }; + const toolsResult = await client.request(toolsRequest, ListToolsResultSchema); + + const tools: { name: string; description?: string; }[] = []; + for (const tool of toolsResult.tools) { + tools.push({ + name: tool.name, + description: tool.description, + }); + } + return tools; + } + + export async function startNotificationTool(client: Client) { + // Call the notification tool using reasonable defaults + const request: CallToolRequest = { + method: 'tools/call', + params: { + name: 'start-notification-stream', + arguments: { + interval: 1000, // 1 second between notifications + count: 5, // Send 5 notifications + }, + }, + }; + const result = await client.request(request, CallToolResultSchema); + + const notifications: { text: string }[] = []; + + result.content.forEach(item => { + if (item.type === 'text') { + notifications.push({ + text: item.text, + }); + } else { + notifications.push({ + text: item.data ? item.data.toString() : '', + }); + } + }); + return notifications; + }Then update both test files to import from this shared utility:
- async function listTools(client: Client) { ... } - async function startNotificationTool(client: Client) { ... } + import { listTools, startNotificationTool } from '../../utils/mcp-test-utils';
58-58: Add a comment explaining the Node.js version check.The test skipping based on Node.js version lacks a comment explaining why version 17 is the threshold. Add a comment to clarify why these tests don't run on Node.js versions 17 and below.
- if (parseInt(process.version.slice(1, 3)) > 17) { + // Skip tests on Node.js <= 17 as MCP features require Node.js >= 18 for native fetch API and other modern features + if (parseInt(process.version.slice(1, 3)) > 17) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/controller/test/mcp/mcp.test.ts(1 hunks)plugin/mcp-proxy/package.json(1 hunks)plugin/mcp-proxy/test/proxy.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/mcp-proxy/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/test/mcp/mcp.test.ts
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
plugin/mcp-proxy/test/proxy.test.ts
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (20)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/controller-decorator/src/util/MCPInfoUtil.ts (2)
86-94:⚠️ Potential issueCritical bug: Prompt params are stored in resource params metadata
The
setMCPPromptParamsandgetMCPPromptParamsmethods currently useCONTROLLER_MCP_RESOURCE_PARAMS_MAPinstead of a dedicated prompt params map. This will cause prompt metadata to overwrite resource metadata or vice versa.static setMCPPromptParams(params: MCPPromptParams & { mcpName?: string }, clazz: EggProtoImplClass, resourceName: string) { - const methodMap: MCPPromptMap = MetadataUtil.initOwnMapMetaData(CONTROLLER_MCP_RESOURCE_PARAMS_MAP, clazz, new Map()); + const methodMap: MCPPromptMap = MetadataUtil.initOwnMapMetaData(CONTROLLER_MCP_PROMPT_PARAMS_MAP, clazz, new Map()); methodMap.set(resourceName, params); } static getMCPPromptParams(clazz: EggProtoImplClass, resourceName: string): MCPPromptParams & { mcpName?: string } | undefined { - const methodMap: MCPPromptMap | undefined = MetadataUtil.getMetaData(CONTROLLER_MCP_RESOURCE_PARAMS_MAP, clazz); + const methodMap: MCPPromptMap | undefined = MetadataUtil.getMetaData(CONTROLLER_MCP_PROMPT_PARAMS_MAP, clazz); return methodMap?.get(resourceName); }If
CONTROLLER_MCP_PROMPT_PARAMS_MAPdoesn't exist yet, it needs to be added to@eggjs/tegg-types.
1-1: 🛠️ Refactor suggestionConsider importing the CONTROLLER_MCP_PROMPT_PARAMS_MAP constant
The imported constants list is missing
CONTROLLER_MCP_PROMPT_PARAMS_MAPwhich would be needed for the fix to the prompt params methods.-import { CONTROLLER_MCP_NAME, CONTROLLER_MCP_PROMPT_MAP, CONTROLLER_MCP_RESOURCE_MAP, CONTROLLER_MCP_RESOURCE_PARAMS_MAP, CONTROLLER_MCP_TOOL_MAP, CONTROLLER_MCP_TOOL_PARAMS_MAP, CONTROLLER_MCP_VERSION, MCPPromptParams, MCPResourceParams, MCPToolParams, EggProtoImplClass } from '@eggjs/tegg-types'; +import { CONTROLLER_MCP_NAME, CONTROLLER_MCP_PROMPT_MAP, CONTROLLER_MCP_PROMPT_PARAMS_MAP, CONTROLLER_MCP_RESOURCE_MAP, CONTROLLER_MCP_RESOURCE_PARAMS_MAP, CONTROLLER_MCP_TOOL_MAP, CONTROLLER_MCP_TOOL_PARAMS_MAP, CONTROLLER_MCP_VERSION, MCPPromptParams, MCPResourceParams, MCPToolParams, EggProtoImplClass } from '@eggjs/tegg-types';
🧹 Nitpick comments (9)
core/controller-decorator/src/decorator/mcp/MCPController.ts (4)
8-8: Consider renaming the "constructor" parameter to avoid shadowing the global propertyThe parameter name
constructorshadows the globalconstructorproperty. While this is a common pattern in TypeScript decorators, consider renaming it to something liketargetorclazzfor better code clarity.- return function(constructor: EggProtoImplClass) { + return function(clazz: EggProtoImplClass) {🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
18-24: Remove commented file paths that appear to be debugging informationThese commented-out file paths don't provide value to the codebase and should be removed.
- // './tegg/core/common-util/src/StackUtil.ts', - // './tegg/core/core-decorator/src/decorator/Prototype.ts', - // './tegg/core/controller-decorator/src/decorator/tr/TRController.ts', - // './tegg/core/core-decorator/node_modules/[email protected]@reflect-metadata/Reflect.js', - // './tegg/core/core-decorator/node_modules/[email protected]@reflect-metadata/Reflect.js', - // './tegg/core/controller-decorator/test/fixtures/TRFooController.ts', PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5));
24-24: Document or refactor the hardcoded stack frame indexThe hardcoded value
5inStackUtil.getCalleeFromStack(false, 5)makes the code brittle if the call stack structure changes. Consider adding a comment explaining why this specific value is used or refactor to calculate it dynamically.- PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5)); + // Skip 5 frames: decorator function, decorator application, internal reflections, etc. + PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5));
5-5: Use consistent import path styleThe import path
../../../src/util/MCPInfoUtilis unusual and might cause issues. Consider using a more direct import path.-import MCPInfoUtil from '../../../src/util/MCPInfoUtil'; +import MCPInfoUtil from '../../util/MCPInfoUtil';core/controller-decorator/src/util/MCPInfoUtil.ts (5)
63-66: Use consistent parameter namingThe parameter
resourceNameis used here but this method deals with tools, not resources. Consider renaming for consistency.- static getMCPToolParams(clazz: EggProtoImplClass, resourceName: string): MCPToolParams & { mcpName?: string } | undefined { + static getMCPToolParams(clazz: EggProtoImplClass, toolName: string): MCPToolParams & { mcpName?: string } | undefined { const methodMap: MCPToolMap | undefined = MetadataUtil.getMetaData(CONTROLLER_MCP_TOOL_PARAMS_MAP, clazz); - return methodMap?.get(resourceName); + return methodMap?.get(toolName); }
68-71: Use consistent parameter namingSimilar to the previous comment,
resourceNameshould be renamed totoolNamefor clarity and consistency.- static setMCPToolParams(params: MCPToolParams & { mcpName?: string }, clazz: EggProtoImplClass, resourceName: string) { + static setMCPToolParams(params: MCPToolParams & { mcpName?: string }, clazz: EggProtoImplClass, toolName: string) { const methodMap: MCPToolMap = MetadataUtil.initOwnMapMetaData(CONTROLLER_MCP_TOOL_PARAMS_MAP, clazz, new Map()); - methodMap.set(resourceName, params); + methodMap.set(toolName, params); }
91-94: Use consistent parameter namingThe parameter
resourceNameis used but this method deals with prompts, not resources. Consider renaming for consistency.- static getMCPPromptParams(clazz: EggProtoImplClass, resourceName: string): MCPPromptParams & { mcpName?: string } | undefined { + static getMCPPromptParams(clazz: EggProtoImplClass, promptName: string): MCPPromptParams & { mcpName?: string } | undefined { const methodMap: MCPPromptMap | undefined = MetadataUtil.getMetaData(CONTROLLER_MCP_RESOURCE_PARAMS_MAP, clazz); - return methodMap?.get(resourceName); + return methodMap?.get(promptName); }
86-89: Use consistent parameter namingSimilar to the previous comment,
resourceNameshould be renamed topromptNamefor clarity and consistency.- static setMCPPromptParams(params: MCPPromptParams & { mcpName?: string }, clazz: EggProtoImplClass, resourceName: string) { + static setMCPPromptParams(params: MCPPromptParams & { mcpName?: string }, clazz: EggProtoImplClass, promptName: string) { const methodMap: MCPPromptMap = MetadataUtil.initOwnMapMetaData(CONTROLLER_MCP_RESOURCE_PARAMS_MAP, clazz, new Map()); - methodMap.set(resourceName, params); + methodMap.set(promptName, params); }
9-96: Consider refactoring to use namespace or individual functionsThe static analysis hints that this class only contains static methods. Consider refactoring to use a namespace or individual functions for better maintainability.
Instead of a class with static methods, you could use a namespace:
export namespace MCPInfoUtil { export function setMCPName(name: string, clazz: EggProtoImplClass) { // implementation } // Other methods... }Or individual exported functions:
export function setMCPName(name: string, clazz: EggProtoImplClass) { // implementation } export function getMCPName(clazz: EggProtoImplClass): string | undefined { // implementation } // Other functions...🧰 Tools
🪛 Biome (1.9.4)
[error] 9-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/controller-decorator/src/decorator/mcp/MCPController.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts(1 hunks)core/controller-decorator/src/model/MCPResourceMeta.ts(1 hunks)core/controller-decorator/src/util/MCPInfoUtil.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/controller-decorator/src/model/MCPResourceMeta.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts
🧰 Additional context used
🪛 Biome (1.9.4)
core/controller-decorator/src/decorator/mcp/MCPController.ts
[error] 8-8: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
core/controller-decorator/src/util/MCPInfoUtil.ts
[error] 9-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (16)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
plugin/mcp-proxy/index.ts (1)
112-141: SSE response is buffered – streaming lost
See earlier review; the body is consumed withresp.text()which disables true Server-Sent-Events streaming.Refactor to pipe the readable body to
ctx.res(or reusehandleSseStream).🧰 Tools
🪛 Biome (1.9.4)
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (1)
164-170: Redundantconst self = thisaliasSame issue as in the register class; can be simplified.
🧰 Tools
🪛 Biome (1.9.4)
[error] 164-164: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
🧹 Nitpick comments (5)
plugin/mcp-proxy/index.ts (2)
59-73: Type safety gap insetProxyHandler
proxyHandlerMapis declared ashandleRequestonly, but for SSE you store ahandlePostMessagecallback which has a different signature.
Either widen the map type or overload the method so TS will catch accidental mis-wiring.- private proxyHandlerMap: { [P in ProxyAction]?: StreamableHTTPServerTransport['handleRequest']; } = {}; + private proxyHandlerMap: { + MCP_STDIO_PROXY?: StreamableHTTPServerTransport['handleRequest']; + MCP_SSE_PROXY?: SSEServerTransport['handlePostMessage']; + MCP_STREAM_PROXY?: StreamableHTTPServerTransport['handleRequest']; + } = {};
134-135:deleteon hot path – minor perf nit
delete headers['transfer-encoding']creates hidden-class churn.
Assigningundefinedor filtering keys into a new object avoids that.
Not critical, just a micro-optimisation.Also applies to: 174-175
🧰 Tools
🪛 Biome (1.9.4)
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
71-92: Unnecessaryself = thisaliasing – clutters closuresArrow functions already capture lexical
this; the extra alias adds noise and risks accidental divergence.Replace
const self = thiswith directthisreferences throughoutmcpStreamServerInit(and the other init helpers).🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (2)
129-154: Potential protocol confusion between SSE and STREAM
transportsis a single shared record that stores both SSE and Streamable transports.
Down-casting withinstanceofworks, but the samesessionIdcould theoretically collide across protocols, leading to class-mismatch 400 errors.Add a namespaced key, e.g.
${protocol}:${sessionId}, or maintain two separate maps.
232-305: Large JSON body parsing without timeout/size guard
getRawBodyis called with 4 MB limit but no request-timeout.
A slow-loris client could keep the connection open and tie up the worker.Set an absolute timeout:
- limit: '4mb', + limit: '4mb', + timeout: 10_000, // 10 s safeguard🧰 Tools
🪛 Biome (1.9.4)
[error] 250-250: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/mcp-proxy/app/extend/agent.ts(1 hunks)plugin/mcp-proxy/app/extend/application.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/mcp-proxy/app/extend/agent.ts
- plugin/mcp-proxy/app/extend/application.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts
[error] 164-164: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 250-250: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 73-73: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 208-208: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 285-285: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 340-340: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 375-375: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 398-398: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/mcp-proxy/index.ts
[error] 134-134: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 174-174: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 GitHub Check: CodeQL
plugin/mcp-proxy/index.ts
[failure] 53-53: Unvalidated dynamic method call
Invocation of method with user-controlled name may dispatch to unexpected target and cause an exception.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
🔇 Additional comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
291-305:transports[sessionId]may beundefined– null-check before useIf the session was never initialised (or was already closed)
handlePostMessagewill throw.- await self.transports[sessionId].handlePostMessage(ctx.req, ctx.res); + const t = self.transports[sessionId]; + if (!t) { + ctx.status = 400; + ctx.body = { error: 'Unknown sessionId' }; + return; + } + await t.handlePostMessage(ctx.req, ctx.res);
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (15)
plugin/mcp-proxy/test/proxy.test.ts (3)
61-63:⚠️ Potential issueRemove duplicate
afterhook.There are two identical
afterhooks that close the application. This duplicates cleanup logic and triggers thenoDuplicateTestHookslinting error.after(async () => { await app.close(); }); afterEach(() => { // mm.restore(); }); before(async () => { mm(process.env, 'EGG_TYPESCRIPT', true); mm(process, 'cwd', () => { return path.join(__dirname, '..'); }); app = mm.cluster({ baseDir: path.join(__dirname, 'fixtures/apps/mcp-proxy'), framework: path.dirname(require.resolve('egg')), // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore workers: 3, sticky: false, opt: { env: { ...process.env, NODE_OPTIONS: '--require ts-node/register tsconfig-paths/register', }, }, }); await app.ready(); }); - after(() => { - return app.close(); - });Also applies to: 91-93
48-48: 🛠️ Refactor suggestionAdd proper null checking instead of using non-null assertion.
Using the non-null assertion operator (
!) can lead to runtime errors ifitem.datais actually null.- text: item.data!.toString(), + text: item.data ? item.data.toString() : '',
95-160: 🛠️ Refactor suggestionDRY: Extract common test logic between SSE and streamable tests.
Both "sse should work" and "streamable should work" tests contain nearly identical code with minor differences in transport types. Extract this common logic to improve maintainability.
+ async function testTransport(transportType, createTransport, getBaseUrl) { + const client = new Client({ + name: `${transportType}-demo-client`, + version: '1.0.0', + }); + const baseUrl = await getBaseUrl(); + const transport = createTransport(new URL(baseUrl)); + const notifications = []; + client.setNotificationHandler(LoggingMessageNotificationSchema, notification => { + notifications.push({ level: notification.params.level, data: notification.params.data as string }); + }); + await client.connect(transport); + const tools = await listTools(client); + const notificationResp = await startNotificationTool(client); + await new Promise(resolve => setTimeout(resolve, 5000)); + + if (transportType === 'streamable') { + await transport.terminateSession(); + await client.close(); + } else { + await transport.close(); + } + + assert.deepEqual(tools, [ + { + name: 'start-notification-stream', + description: 'Starts sending periodic notifications for testing resumability', + }, + ]); + assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); + assert.deepEqual(notifications, [ + { level: 'info', data: 'Periodic notification #1' }, + { level: 'info', data: 'Periodic notification #2' }, + { level: 'info', data: 'Periodic notification #3' }, + { level: 'info', data: 'Periodic notification #4' }, + { level: 'info', data: 'Periodic notification #5' }, + ]); + } it('sse should work', async () => { - const sseClient = new Client({ - name: 'sse-demo-client', - version: '1.0.0', - }); - const baseUrl = await app.httpRequest() - .get('/init').url; - const sseTransport = new SSEClientTransport(new URL(baseUrl)); - const sseNotifications: { level: string, data: string }[] = []; - sseClient.setNotificationHandler(LoggingMessageNotificationSchema, notification => { - sseNotifications.push({ level: notification.params.level, data: notification.params.data as string }); - }); - await sseClient.connect(sseTransport); - const tools = await listTools(sseClient); - const notificationResp = await startNotificationTool(sseClient); - await new Promise(resolve => setTimeout(resolve, 5000)); - await sseTransport.close(); - assert.deepEqual(tools, [ - { - name: 'start-notification-stream', - description: 'Starts sending periodic notifications for testing resumability', - }, - ]); - assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); - assert.deepEqual(sseNotifications, [ - { level: 'info', data: 'Periodic notification #1' }, - { level: 'info', data: 'Periodic notification #2' }, - { level: 'info', data: 'Periodic notification #3' }, - { level: 'info', data: 'Periodic notification #4' }, - { level: 'info', data: 'Periodic notification #5' }, - ]); + await testTransport( + 'sse', + url => new SSEClientTransport(url), + async () => await app.httpRequest().get('/init').url + ); });plugin/controller/test/mcp/mcp.test.ts (3)
61-63:⚠️ Potential issueRemove duplicate
afterhook.There are two identical
afterhooks that close the application. This duplicates cleanup logic and triggers thenoDuplicateTestHookslinting error.after(async () => { await app.close(); }); afterEach(() => { // mm.restore(); }); before(async () => { mm(process.env, 'EGG_TYPESCRIPT', true); mm(process, 'cwd', () => { return path.join(__dirname, '../..'); }); app = mm.cluster({ baseDir: path.join(__dirname, '../fixtures/apps/mcp-app'), framework: path.dirname(require.resolve('egg')), // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore workers: 3, sticky: false, opt: { env: { ...process.env, NODE_OPTIONS: '--require ts-node/register tsconfig-paths/register', }, }, }); await app.ready(); }); - after(() => { - return app.close(); - });Also applies to: 91-93
48-48: 🛠️ Refactor suggestionAdd proper null checking instead of using non-null assertion.
Using the non-null assertion operator (
!) can lead to runtime errors ifitem.datais actually null.- text: item.data!.toString(), + text: item.data ? item.data.toString() : '',
95-277: 🛠️ Refactor suggestionDRY: Extract common test logic between SSE and streamable tests.
Both test cases contain duplicate code with only minor differences. Creating a shared test function would improve maintainability and make the tests more concise.
+ async function testMCPTransport( + transportType: 'sse' | 'streamable', + createTransport: (url: URL) => SSEClientTransport | StreamableHTTPClientTransport, + getEndpointUrl: () => Promise<string> + ) { + const client = new Client({ + name: `${transportType}-demo-client`, + version: '1.0.0', + }); + const baseUrl = await getEndpointUrl(); + const transport = createTransport(new URL(baseUrl)); + const notifications: { level: string, data: string }[] = []; + client.setNotificationHandler(LoggingMessageNotificationSchema, notification => { + notifications.push({ level: notification.params.level, data: notification.params.data as string }); + }); + await client.connect(transport); + + // Test tools + const tools = await listTools(client); + assert.deepEqual(tools, [ + { + name: 'start-notification-stream', + description: 'Starts sending periodic notifications for testing resumability', + }, + { + description: undefined, + name: 'bar', + }, + ]); + + const toolRes = await client.callTool({ + name: 'bar', + arguments: { + name: 'aaa', + }, + }); + assert.deepEqual(toolRes, { + content: [{ type: 'text', text: 'npm package: aaa not found' }], + }); + + // Test notifications + const notificationResp = await startNotificationTool(client); + await new Promise(resolve => setTimeout(resolve, 5000)); + assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); + assert.deepEqual(notifications, [ + { level: 'info', data: 'Periodic notification #1' }, + { level: 'info', data: 'Periodic notification #2' }, + { level: 'info', data: 'Periodic notification #3' }, + { level: 'info', data: 'Periodic notification #4' }, + { level: 'info', data: 'Periodic notification #5' }, + ]); + + // Test resources + const resources = await client.listResources(); + assert.deepEqual(resources, { + resources: [ + { uri: 'mcp://npm/egg?version=4.10.0', name: 'egg' }, + { uri: 'mcp://npm/mcp?version=0.10.0', name: 'mcp' }, + ], + }); + + const resourceRes = await client.readResource({ + uri: 'mcp://npm/egg?version=4.10.0', + }); + assert.deepEqual(resourceRes, { + contents: [{ uri: 'mcp://npm/egg?version=4.10.0', text: 'MOCK TEXT' }], + }); + + // Test prompts + const prompts = await client.listPrompts(); + assert.deepEqual(prompts, { + prompts: [ + { name: 'foo', arguments: [{ name: 'name', required: true }] }, + ], + }); + + const promptRes = await client.getPrompt({ + name: 'foo', + arguments: { + name: 'bbb', + }, + }); + assert.deepEqual(promptRes, { + messages: [ + { + role: 'user', + content: { + type: 'text', + text: 'Generate a concise but descriptive commit message for these changes:\n\nbbb', + }, + }, + ], + }); + + // Close transport appropriately based on type + if (transportType === 'sse') { + await (transport as SSEClientTransport).close(); + } else { + await (transport as StreamableHTTPClientTransport).terminateSession(); + await client.close(); + } + } it('sse should work', async () => { - const sseClient = new Client({ - name: 'sse-demo-client', - version: '1.0.0', - }); - const baseUrl = await app.httpRequest() - .get('/mcp/init').url; - const sseTransport = new SSEClientTransport(new URL(baseUrl)); - const sseNotifications: { level: string, data: string }[] = []; - sseClient.setNotificationHandler(LoggingMessageNotificationSchema, notification => { - sseNotifications.push({ level: notification.params.level, data: notification.params.data as string }); - }); - await sseClient.connect(sseTransport); - // tool - const tools = await listTools(sseClient); - assert.deepEqual(tools, [ - { - name: 'start-notification-stream', - description: 'Starts sending periodic notifications for testing resumability', - }, - { - description: undefined, - name: 'bar', - }, - ]); - - const toolRes = await sseClient.callTool({ - name: 'bar', - arguments: { - name: 'aaa', - }, - }); - assert.deepEqual(toolRes, { - content: [{ type: 'text', text: 'npm package: aaa not found' }], - }); - // notification - const notificationResp = await startNotificationTool(sseClient); - await new Promise(resolve => setTimeout(resolve, 5000)); - assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); - assert.deepEqual(sseNotifications, [ - { level: 'info', data: 'Periodic notification #1' }, - { level: 'info', data: 'Periodic notification #2' }, - { level: 'info', data: 'Periodic notification #3' }, - { level: 'info', data: 'Periodic notification #4' }, - { level: 'info', data: 'Periodic notification #5' }, - ]); - - // resources - const resources = await sseClient.listResources(); - assert.deepEqual(resources, { - resources: [ - { uri: 'mcp://npm/egg?version=4.10.0', name: 'egg' }, - { uri: 'mcp://npm/mcp?version=0.10.0', name: 'mcp' }, - ], - }); - - const resourceRes = await sseClient.readResource({ - uri: 'mcp://npm/egg?version=4.10.0', - }); - assert.deepEqual(resourceRes, { - contents: [{ uri: 'mcp://npm/egg?version=4.10.0', text: 'MOCK TEXT' }], - }); - - // prompts - const prompts = await sseClient.listPrompts(); - assert.deepEqual(prompts, { - prompts: [ - { name: 'foo', arguments: [{ name: 'name', required: true }] }, - ], - }); - - const promptRes = await sseClient.getPrompt({ - name: 'foo', - arguments: { - name: 'bbb', - }, - }); - assert.deepEqual(promptRes, { - messages: [ - { - role: 'user', - content: { - type: 'text', - text: 'Generate a concise but descriptive commit message for these changes:\n\nbbb', - }, - }, - ], - }); - await sseTransport.close(); + await testMCPTransport( + 'sse', + url => new SSEClientTransport(url), + async () => await app.httpRequest().get('/mcp/init').url + ); }); it('streamable should work', async () => { - const streamableClient = new Client({ - name: 'streamable-demo-client', - version: '1.0.0', - }); - const baseUrl = await app.httpRequest() - .post('/mcp/stream').url; - const streamableTransport = new StreamableHTTPClientTransport(new URL(baseUrl)); - const streamableNotifications: { level: string, data: string }[] = []; - streamableClient.setNotificationHandler(LoggingMessageNotificationSchema, notification => { - streamableNotifications.push({ level: notification.params.level, data: notification.params.data as string }); - }); - await streamableClient.connect(streamableTransport); - // tool - const tools = await listTools(streamableClient); - assert.deepEqual(tools, [ - { - name: 'start-notification-stream', - description: 'Starts sending periodic notifications for testing resumability', - }, - { - description: undefined, - name: 'bar', - }, - ]); - - const toolRes = await streamableClient.callTool({ - name: 'bar', - arguments: { - name: 'aaa', - }, - }); - assert.deepEqual(toolRes, { - content: [{ type: 'text', text: 'npm package: aaa not found' }], - }); - // notification - const notificationResp = await startNotificationTool(streamableClient); - await new Promise(resolve => setTimeout(resolve, 5000)); - assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); - assert.deepEqual(streamableNotifications, [ - { level: 'info', data: 'Periodic notification #1' }, - { level: 'info', data: 'Periodic notification #2' }, - { level: 'info', data: 'Periodic notification #3' }, - { level: 'info', data: 'Periodic notification #4' }, - { level: 'info', data: 'Periodic notification #5' }, - ]); - - // resources - const resources = await streamableClient.listResources(); - assert.deepEqual(resources, { - resources: [ - { uri: 'mcp://npm/egg?version=4.10.0', name: 'egg' }, - { uri: 'mcp://npm/mcp?version=0.10.0', name: 'mcp' }, - ], - }); - - const resourceRes = await streamableClient.readResource({ - uri: 'mcp://npm/egg?version=4.10.0', - }); - assert.deepEqual(resourceRes, { - contents: [{ uri: 'mcp://npm/egg?version=4.10.0', text: 'MOCK TEXT' }], - }); - - // prompts - const prompts = await streamableClient.listPrompts(); - assert.deepEqual(prompts, { - prompts: [ - { name: 'foo', arguments: [{ name: 'name', required: true }] }, - ], - }); - - const promptRes = await streamableClient.getPrompt({ - name: 'foo', - arguments: { - name: 'bbb', - }, - }); - assert.deepEqual(promptRes, { - messages: [ - { - role: 'user', - content: { - type: 'text', - text: 'Generate a concise but descriptive commit message for these changes:\n\nbbb', - }, - }, - ], - }); - - await streamableTransport.terminateSession(); - await streamableClient.close(); + await testMCPTransport( + 'streamable', + url => new StreamableHTTPClientTransport(url), + async () => await app.httpRequest().post('/mcp/stream').url + ); });plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (1)
107-145:⚠️ Potential issueFix proxy handler overwriting issue in
onsessioninitialized.The
setProxyHandler('STREAM', ...)call in theonsessioninitializedcallback will overwrite any existing handler for each new session. This breaks earlier sessions in multi-tenant scenarios.- this.app.mcpProxy.setProxyHandler('STREAM', async (req, res) => { - return await self.streamPostHandler(req, res); - });Consider setting up the proxy handler once outside the session initialization, or use a more sophisticated approach that maps handlers to specific session IDs.
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (3)
291-306:⚠️ Potential issueValidate
sessionIdbefore using it.The
sessionIdobtained fromctx.query.sessionIdis not validated before use, potentially causing runtime errors if it's undefined.- const sessionId = ctx.query.sessionId; + const sessionId = String(ctx.query.sessionId ?? ''); + if (!sessionId) { + ctx.status = 400; + ctx.body = { error: 'sessionId required' }; + return; + } const detail = await self.app.mcpProxy.getClient(sessionId);
429-437:⚠️ Potential issueFix initialization order to prevent race conditions.
The
mcpStreamServerInit,mcpServerInit, andmcpServerRegistermethods are called immediately after creating the server but before completing async operations. This creates a potential race condition ifregister()is called concurrently.Consider either:
- Making the initialization methods return promises and awaiting them sequentially, or
- Implementing a mutex to ensure atomic initialization
107-144:⚠️ Potential issueFix proxy handler overwriting issue in
onsessioninitialized.Each new session registers a new handler for the 'STREAM' protocol, overwriting any previous handlers. This will break earlier sessions in multi-tenant scenarios.
Consider either:
- Moving the
setProxyHandlercall outside the per-session callback, or- Storing handlers in a map keyed by both protocol and sessionId
plugin/mcp-proxy/index.ts (5)
21-21:⚠️ Potential issueTypo in
ProxyActiontype definitionThe type definition includes
'MCP_SEE_PROXY'which should be'MCP_SSE_PROXY'(for Server-Sent Events).-type ProxyAction = 'MCP_STDIO_PROXY' | 'MCP_SEE_PROXY' | 'MCP_STREAM_PROXY'; +type ProxyAction = 'MCP_STDIO_PROXY' | 'MCP_SSE_PROXY' | 'MCP_STREAM_PROXY';
37-37:⚠️ Potential issue
_clientproperty never initialized – all delegated calls will throwThe subclass redeclares
private _client: anybut never assigns it, causing all delegate methods to fail.- private _client: any;
54-55:⚠️ Potential issuePort listener initialization needs error handling
Variable name is incorrect and the server.listen call lacks error handling.
- const post = 17031 + (cluster.worker?.id ?? 0); - await new Promise(resolve => server.listen(post, () => { resolve(null); })); + const port = 17031 + (cluster.worker?.id ?? 0); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(port, () => resolve(null)); + });
62-62:⚠️ Potential issueTypo in SSE proxy action name
The same typo as in the type definition appears here.
- action = 'MCP_SEE_PROXY'; + action = 'MCP_SSE_PROXY';
115-140:⚠️ Potential issueSSE response is read fully – streaming is lost
The SSE implementation reads the entire response body as text and sends it back as one chunk, defeating the purpose of SSE where the client expects a stream of events.
case 'SSE': { - action = 'MCP_SEE_PROXY'; + action = 'MCP_SSE_PROXY'; ctx.req.headers['mcp-proxy-type'] = action; ctx.req.headers['mcp-proxy-sessionid'] = sessionId; const resp = await fetch(`http://localhost:${17031 + detail.workerId}/mcp/message?sessionId=${sessionId}`, { headers: ctx.req.headers as unknown as Record<string, string>, body: body as string, method: ctx.req.method, }); const headers: Record<string, string> = {}; for (const [ key, value ] of resp.headers.entries()) { headers[key] = value; } delete headers['transfer-encoding']; ctx.set(headers); ctx.res.statusCode = resp.status; ctx.res.statusMessage = resp.statusText; - const resData = await resp.text(); - ctx.body = resData; + ctx.respond = false; + Readable.fromWeb(resp.body!).pipe(ctx.res); break; }🧰 Tools
🪛 Biome (1.9.4)
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🧹 Nitpick comments (9)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (1)
164-164: Remove unnecessarythisaliasing.Arrow functions inherit
thisfrom their enclosing scope. Theselfvariable is redundant.- const self = this; const transport = new SSEServerTransport('/message', this.ctx.res);- const self = this; const transport = new StreamableHTTPServerTransport({Also applies to: 250-250
🧰 Tools
🪛 Biome (1.9.4)
[error] 164-164: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
73-73: Remove unnecessarythisaliasing throughout the file.Arrow functions inherit
thisfrom their enclosing scope, making theselfvariable redundant. This appears multiple times in the file.- const self = this;Also applies to: 208-208, 285-285, 340-340, 375-375, 398-398
🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
plugin/mcp-proxy/index.ts (7)
112-112: Remove or uncomment socket path configurationThe commented code suggests an alternative implementation using Unix sockets that was replaced with TCP ports.
Either remove this commented code or add a comment explaining why it's being kept for reference.
133-133: Use undefined assignment instead of delete operatorThe delete operator can impact performance.
- delete headers['transfer-encoding']; + headers['transfer-encoding'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
173-173: Use undefined assignment instead of delete operatorThe delete operator can impact performance.
- delete headers['transfer-encoding']; + headers['transfer-encoding'] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 173-173: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
191-197: Clean up TypeScript ignores with proper typesMultiple
@ts-ignorecomments indicate TypeScript compatibility issues that should be properly addressed.Consider creating proper type definitions for these interfaces or using the correct types from the library:
- const reader = stream - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - .pipeThrough(new TextDecoderStream()) - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - .pipeThrough(new EventSourceParserStream()) - .getReader(); + // Cast to proper web streams API types + const reader = (stream as ReadableStream<Uint8Array>) + .pipeThrough(new TextDecoderStream()) + .pipeThrough(new EventSourceParserStream()) + .getReader();
218-218: Correct error message typoThe error message contains a typo.
- ctx.res.write(`see stream error ${error}`); + ctx.res.write(`SSE stream error: ${error}`);
82-84: Add error handling to getClient methodThe method calls
this._client.getClient(sessionId)which could be undefined. Add proper error handling.async getClient(sessionId: string): Promise<ClientDetail | undefined> { - return this._client.getClient(sessionId); + try { + return this._client.getClient(sessionId); + } catch (error) { + this.logger.error(`Failed to get client for session ${sessionId}:`, error); + return undefined; + } }
182-182: Uncomment or replace error response codeThere's a commented-out error response that should be uncommented or replaced with an appropriate error handling.
- // ctx.res.writeHead(400).end(`Invalid message: ${body}`); + ctx.res.writeHead(500).end(`Proxy error: ${error.message}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/controller/test/mcp/mcp.test.ts(1 hunks)plugin/mcp-proxy/app/extend/agent.ts(1 hunks)plugin/mcp-proxy/app/extend/application.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts(1 hunks)plugin/mcp-proxy/test/proxy.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/mcp-proxy/app/extend/agent.ts
- plugin/mcp-proxy/app/extend/application.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 73-73: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 208-208: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 285-285: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 340-340: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 375-375: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 398-398: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/controller/test/mcp/mcp.test.ts
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
plugin/mcp-proxy/index.ts
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 173-173: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts
[error] 164-164: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 250-250: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/mcp-proxy/test/proxy.test.ts
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🪛 GitHub Check: CodeQL
plugin/mcp-proxy/index.ts
[failure] 53-53: Unvalidated dynamic method call
Invocation of method with user-controlled name may dispatch to unexpected target and cause an exception.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (18)
|
README 先写一下用户视角的 usage？ |
|
@coderabbitai 提一个 issue 跟进此 pr 合并后会被 pick 到 next 分支 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@fengmk2 I've created an issue to track picking PR #307 to the next branch after it's merged. The issue has been assigned to you and includes the necessary context for follow-up action. 🏁 Script executed: Length of output: 555 @fengmk2 Great! The issue has been successfully created at #308. The follow-up task to pick PR #307 to the next branch after merging is now tracked and assigned to you. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces comprehensive MCP controller support by adding new decorators, metadata builders, utility functions, and tests to enable MCP operations with Egg.js. Key changes include:
- New MCP decorators (MCPController, MCPPrompt, MCPTool, MCPResource) to mark controller methods.
- Utility and meta builder updates for aggregating MCP metadata.
- Extensive tests that assert proper MCP metadata generation and integration.
Reviewed Changes
Copilot reviewed 54 out of 56 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/types/controller-decorator/MCPController.ts | New type definitions for MCP controllers and hooks. |
| core/controller-decorator/test/fixtures/MCPController.ts | Example MCP controller with prompt, tool, and resource implementations. |
| core/controller-decorator/test/MCPMeta.test.ts | Test suite ensuring that MCP metadata is correctly generated. |
| core/controller-decorator/src/util/MCPInfoUtil.ts | Utility methods for setting/getting MCP metadata. |
| core/controller-decorator/src/model/* | New MCP metadata models for controllers, prompts, tools, and resources. |
| core/controller-decorator/src/impl/mcp/* | Meta builder implementations for assembling MCP metadata. |
| core/controller-decorator/src/decorator/mcp/* | New MCP decorators for controllers and related methods. |
| core/controller-decorator/index.ts | Updated exports to include new MCP components. |
Files not reviewed (2)
- .eslintrc: Language not supported
- core/controller-decorator/package.json: Language not supported
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/controller-decorator/src/util/MCPInfoUtil.ts (1)
86-94:⚠️ Potential issuePrompt params are still stored under the resource key – metadata collision
Previous reviews already flagged this:
setMCPPromptParams/getMCPPromptParamspersist prompt metadata inCONTROLLER_MCP_RESOURCE_PARAMS_MAP, causing prompts & resources to overwrite each other.Please switch to a dedicated metadata key (e.g.
CONTROLLER_MCP_PROMPT_PARAMS_MAP) just like tools have their own.-import { ..., CONTROLLER_MCP_RESOURCE_PARAMS_MAP, ... } +import { ..., CONTROLLER_MCP_PROMPT_PARAMS_MAP, CONTROLLER_MCP_RESOURCE_PARAMS_MAP, ... } @@ - const methodMap: MCPPromptMap = MetadataUtil.initOwnMapMetaData(CONTROLLER_MCP_RESOURCE_PARAMS_MAP, clazz, new Map()); + const methodMap: MCPPromptMap = MetadataUtil.initOwnMapMetaData(CONTROLLER_MCP_PROMPT_PARAMS_MAP, clazz, new Map()); @@ - const methodMap: MCPPromptMap | undefined = MetadataUtil.getMetaData(CONTROLLER_MCP_RESOURCE_PARAMS_MAP, clazz); + const methodMap: MCPPromptMap | undefined = MetadataUtil.getMetaData(CONTROLLER_MCP_PROMPT_PARAMS_MAP, clazz);
🧹 Nitpick comments (4)
core/controller-decorator/src/decorator/mcp/MCPController.ts (2)
18-23: Remove stray stack-trace commentsLines 18-23 contain what look like pasted stack-trace snippets.
They add no value, clutter the source, and will confuse future readers.- // './tegg/core/common-util/src/StackUtil.ts', - // './tegg/core/core-decorator/src/decorator/Prototype.ts', - // './tegg/core/controller-decorator/src/decorator/tr/TRController.ts', - // './tegg/core/core-decorator/node_modules/[email protected]@reflect-metadata/Reflect.js', - // './tegg/core/core-decorator/node_modules/[email protected]@reflect-metadata/Reflect.js', - // './tegg/core/controller-decorator/test/fixtures/TRFooController.ts',
24-24: Magic number for stack depth obscures intent
StackUtil.getCalleeFromStack(false, 5)hard-codes the depth.
Consider extracting5into a named constant (e.g.CALLER_STACK_DEPTH) or adding a short explanatory comment so future refactors don’t break file-path detection.core/types/controller-decorator/MCPController.ts (1)
17-24: Hook naming is inconsistent and may confuse users
preHandleInitHandle(line 19) is the only hook whose name ends with “InitHandle”.
All others follow a simplerpreSomething/onSomethingpattern.
Consider renaming to eitherpreHandleInitor clarifying its purpose in JSDoc so implementers don’t wonder which phase it runs in.core/controller-decorator/src/util/MCPInfoUtil.ts (1)
9-10: Static-only utility class – consider converting to a module of free functions
MCPInfoUtilonly hosts static methods, triggering thenoStaticOnlyClasslint rule.
While not critical, replacing it with plain named exports can reduce TS output size and avoid accidental instantiation.No change required if you prefer the current style; just be aware of the rule.
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/controller-decorator/index.ts(2 hunks)core/controller-decorator/src/decorator/mcp/MCPController.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPPrompt.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPResource.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPTool.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerResourceMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerToolMetaBuilder.ts(1 hunks)core/controller-decorator/src/util/MCPInfoUtil.ts(1 hunks)core/controller-decorator/test/MCPMeta.test.ts(1 hunks)core/types/controller-decorator/MCPController.ts(1 hunks)core/types/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- core/types/package.json
- core/controller-decorator/src/decorator/mcp/MCPResource.ts
- core/controller-decorator/src/impl/mcp/MCPControllerResourceMetaBuilder.ts
- core/controller-decorator/src/decorator/mcp/MCPPrompt.ts
- core/controller-decorator/src/impl/mcp/MCPControllerToolMetaBuilder.ts
- core/controller-decorator/src/decorator/mcp/MCPTool.ts
- core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts
- core/controller-decorator/index.ts
- core/controller-decorator/test/MCPMeta.test.ts
- core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/types/controller-decorator/MCPController.ts (4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
register(409-457)MCPControllerRegister(34-458)core/controller-decorator/src/model/MCPControllerMeta.ts (1)
MCPControllerMeta(7-70)core/controller-decorator/src/model/MCPPromptMeta.ts (1)
MCPPromptMeta(4-30)core/controller-decorator/src/model/MCPToolMeta.ts (1)
MCPToolMeta(5-31)
🪛 Biome (1.9.4)
core/controller-decorator/src/decorator/mcp/MCPController.ts
[error] 8-8: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
core/controller-decorator/src/util/MCPInfoUtil.ts
[error] 9-96: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Socket Security: Pull Request Alerts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (3)
185-188: Add proper type checking fordetailbefore using non-null assertion.Using non-null assertion (
!) ondetailis risky ifdetailis null or undefined. Adding proper null checking is safer.const detail = await this.app.mcpProxy.getClient(this.ctx.request.query.sessionId); if (detail?.pid !== process.pid) { await this.app.mcpProxy.proxyMessage(this.ctx, { - detail: detail!, + detail, sessionId: this.ctx.request.query.sessionId, type: MCPProtocols.SSE, }); } else { await this.ssePostHandler(this.ctx.req, this.ctx.res); }
260-265: Add proper null checking forsidbefore using non-null assertion.Using non-null assertion (
!) onsidis risky ifsidis null or undefined. Add proper type checking instead.const sid = transport.sessionId; if (sid && transports[sid]) { delete transports[sid]; + await this.app.mcpProxy.unregisterClient(sid); + } else if (sid) { + await this.app.mcpProxy.unregisterClient(sid); } - await this.app.mcpProxy.unregisterClient(sid!);
283-288: 🛠️ Refactor suggestionAdd proper type checking for
detailbefore using non-null assertion.Using non-null assertion (
!) ondetailin line 285 is risky ifdetailis null or undefined. Adding proper null checking is safer.const detail = await this.app.mcpProxy.getClient(sessionId); if (detail?.pid !== process.pid) { await this.app.mcpProxy.proxyMessage(this.ctx, { - detail: detail!, + detail, sessionId, type: MCPProtocols.STREAM, }); } else { this.ctx.respond = false;plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
349-357:⚠️ Potential issueInitialisation order – router hooks may run before
mcpServerready.
mcpStreamServerInit,mcpServerInit, andmcpServerRegisterrely onthis.mcpServerbeing non-null, yet they are called immediately after creation and before awaiting any async work. If later code callsregister()concurrently, there's a race.if (!this.mcpServer) { this.mcpServer = new McpServer({ name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, version: this.controllerMeta.version ?? '1.0.0', }, { capabilities: { logging: {} } }); - this.mcpStreamServerInit(); - this.mcpServerInit(); - this.mcpServerRegister(); + // Initialize mcpServer fully before registering routes + await this.mcpStreamServerInit(); + this.mcpServerInit(); + this.mcpServerRegister(); }
222-239:⚠️ Potential issueValidate
sessionIdto prevent crashes when accessing undefined.
sessionIdis used directly fromctx.querywithout validation. This can lead to errors when trying to access a transport with an undefined key or when passing undefined togetClient().- const sessionId = ctx.query.sessionId; + const sessionId = ctx.query.sessionId as string; + if (!sessionId) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32000, + message: 'Bad Request: Missing sessionId parameter', + }, + id: null, + }; + return; + } if (MCPControllerRegister.hooks.length > 0) {plugin/mcp-proxy/index.ts (2)
189-192:⚠️ Potential issueEnforce validation of
mcp-proxy-typebefore dispatching proxy handlers.The current code casts the untrusted header directly to
ProxyActionand invokes the handler without checking whether it's one of the expected values. This can lead to silent failures or runtime errors.const server = http.createServer(async (req, res) => { - const type = req.headers['mcp-proxy-type'] as ProxyAction; - await this.proxyHandlerMap[type]?.(req, res); + const typeHeader = req.headers['mcp-proxy-type'] as string; + // Validate that the header matches one of the allowed ProxyAction keys + if (!typeHeader || !this.proxyHandlerMap[typeHeader as ProxyAction]) { + res.writeHead(400).end('Invalid or unsupported MCP proxy type'); + return; + } + const action = typeHeader as ProxyAction; + await this.proxyHandlerMap[action]!(req, res); });
194-199: 🛠️ Refactor suggestionAdd proper error handling for server.listen.
The current implementation doesn't handle errors when starting the HTTP server. If the port is already in use, this will silently fail.
this.port = (this.app.config.mcp?.proxyPort ?? 17031) + (cluster.worker?.id ?? 0); this.addProxyHook(); - await new Promise(resolve => server.listen(this.port, () => { + await new Promise((resolve, reject) => { + server.once('error', err => { + this.logger.error('[MCPProxyApiClient] Failed to start HTTP server:', err); + reject(err); + }); + server.listen(this.port, () => { // const address = server.address()! as AddressInfo; // this.port = address.port; resolve(null); - })); + }); + });
🧹 Nitpick comments (5)
plugin/mcp-proxy/app.ts (1)
4-8: Improve type safety in Application reference.The current implementation uses
Applicationfrom egg but refers to it asagentin the property and constructor. This creates confusion asagentandapplicationare distinct concepts in Egg.js.- private readonly agent: Application; + private readonly agent: Application; constructor(agent: Application) { this.agent = agent; }plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (2)
164-165: Unnecessarythisaliasing.Arrow functions inherit
thisfrom their enclosing scope, making theselfvariable unnecessary.- const self = this; const transport = new SSEServerTransport('/message', this.ctx.res);🧰 Tools
🪛 Biome (1.9.4)
[error] 164-164: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
247-248: Unnecessarythisaliasing.Arrow functions inherit
thisfrom their enclosing scope, making theselfvariable unnecessary.- const self = this; const transport = new StreamableHTTPServerTransport({🧰 Tools
🪛 Biome (1.9.4)
[error] 247-247: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
69-70: Remove unnecessarythisaliasing in arrow functions.Arrow functions inherit
thisfrom their enclosing scope, making theselfvariable unnecessary.- const self = this;Apply this change to all similar instances throughout the file (lines 69, 114, 180, 217, etc.)
Also applies to: 114-115, 180-181, 217-218
🧰 Tools
🪛 Biome (1.9.4)
[error] 69-69: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
101-105: Fix grammar in error message.The error message "body should is json" has incorrect grammar.
ctx.body = { jsonrpc: '2.0', error: { code: -32000, - message: 'Bad Request: body should is json', + message: 'Bad Request: body should be JSON', }, id: null, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/types/controller-decorator/MCPController.ts(1 hunks)core/types/package.json(1 hunks)plugin/controller/app.ts(2 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/mcp-proxy/agent.ts(1 hunks)plugin/mcp-proxy/app.ts(1 hunks)plugin/mcp-proxy/app/extend/agent.ts(1 hunks)plugin/mcp-proxy/app/extend/application.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)plugin/mcp-proxy/package.json(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts(1 hunks)plugin/mcp-proxy/typings/index.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugin/mcp-proxy/agent.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- core/types/package.json
- plugin/mcp-proxy/typings/index.d.ts
- plugin/mcp-proxy/app/extend/application.ts
- plugin/controller/app.ts
- plugin/mcp-proxy/app/extend/agent.ts
- plugin/mcp-proxy/package.json
- core/types/controller-decorator/MCPController.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/mcp-proxy/index.ts (3)
core/types/controller-decorator/MCPController.ts (1)
MCPControllerHook(17-30)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
MCPControllerRegister(32-368)plugin/mcp-proxy/lib/MCPProxyDataClient.ts (1)
MCPProxyDataClient(4-39)
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 69-69: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 180-180: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 217-217: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 267-267: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 295-295: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 318-318: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts
[error] 164-164: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 247-247: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-macos (18)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (22)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (3)
87-87: Consider if cache-control is necessary hereThe cache-control header might not be relevant for MCP connections. Verify if it's necessary or if it can be removed to avoid confusion.
236-253:⚠️ Potential issuesessionId is not validated – may crash on undefined
No validation exists for
sessionIdwhich can lead to runtime errors if undefined or invalid.- const sessionId = ctx.query.sessionId; + const sessionId = String(ctx.query.sessionId ?? ''); + if (!sessionId) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32000, + message: 'Bad Request: sessionId required', + }, + id: null, + }; + return; + } + + if (!self.transports[sessionId]) { + ctx.status = 404; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session not found', + }, + id: null, + }; + return; + }
363-370:⚠️ Potential issueInitialisation order – router hooks may run before
mcpServerreadyThe initialization order can create race conditions as the router hooks may execute before
mcpServeris ready.Consider sequentially awaiting the initialization methods:
if (!this.mcpServer) { this.mcpServer = new McpServer({ name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, version: this.controllerMeta.version ?? '1.0.0', }, { capabilities: { logging: {} } }); - this.mcpStreamServerInit(); - this.mcpServerInit(); - this.mcpServerRegister(); + await this.mcpStreamServerInit(); + this.mcpServerInit(); + this.mcpServerRegister(); }
🧹 Nitpick comments (5)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (5)
114-114: Grammar error in error messageThe error message contains a grammatical error.
- message: 'Bad Request: body should is json', + message: 'Bad Request: body should be JSON',
126-130: Allow custom eventStore and sessionIdGenerator implementationsThe code could be simplified while still allowing for custom implementations.
- const eventStore = this.app.config.mcp?.eventStore ?? new InMemoryEventStore(); - const self = this; - const transport = new StreamableHTTPServerTransport({ - sessionIdGenerator: () => this.app.config.mcp?.sessionIdGenerator ?? randomUUID(), - eventStore, + const transport = new StreamableHTTPServerTransport({ + sessionIdGenerator: this.app.config.mcp?.sessionIdGenerator || (() => randomUUID()), + eventStore: this.app.config.mcp?.eventStore || new InMemoryEventStore(),
82-82: Remove unnecessaryselfvariablesArrow functions inherit
thisfrom their enclosing scope, making these aliases unnecessary.- const self = this; const handler = async (...args) => { - const eggObj = await self.eggContainerFactory.getOrCreateEggObject(...); + const eggObj = await this.eggContainerFactory.getOrCreateEggObject(...);Also applies to: 127-127, 193-193, 230-230, 280-280, 308-308, 331-331
🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
215-215: Make heartbeat interval configurableThe default heartbeat interval is hardcoded to 25000ms. Consider extracting this to a constant or default config value for better maintainability.
- }, self.app.config.mcp?.sseHeartTime ?? 25000); + }, self.app.config.mcp?.sseHeartTime ?? self.app.config.mcp?.defaultSseHeartbeatTime ?? 25000);
371-379: Use object destructuring when iterating through Map entriesThe code can be simplified by using destructuring in the map iteration.
- for (const [ prompt, controllerProto ] of promptMap.entries()) { - await this.mcpPromptRegister(controllerProto, prompt); + for (const [prompt, controllerProto] of promptMap) { + await this.mcpPromptRegister(controllerProto, prompt);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/types/controller-decorator/MCPController.ts(1 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/types/controller-decorator/MCPController.ts
- plugin/mcp-proxy/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 82-82: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 193-193: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 230-230: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 280-280: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 308-308: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 331-331: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (4)
95-154:⚠️ Potential issueFix missing return after JSON parse error.
The code continues execution after catching a JSON parse error, leading to potential undefined behavior with uninitialized
bodyvariable. This issue was previously identified in a review but hasn't been addressed.try { body = JSON.parse(rawBody); } catch (e) { ctx.status = 400; ctx.body = { jsonrpc: '2.0', error: { code: -32000, message: 'Bad Request: body should is json', }, id: null, }; + return; }
235-237:⚠️ Potential issueMissing sessionId validation could cause application crash.
The
sessionIdfrom query parameters is used without validation, which could lead to undefined behavior or crashes when accessingself.transports[sessionId]if sessionId is undefined. This was previously flagged in a review but hasn't been addressed.- const sessionId = ctx.query.sessionId; + const sessionId = String(ctx.query.sessionId ?? ''); + if (!sessionId) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32000, + message: 'Bad Request: sessionId required', + }, + id: null, + }; + return; + }
281-288: 🛠️ Refactor suggestionUse type-safe approach for registering handlers.
The code uses unsafe type casting with
as unknown as [any, any, any, any]when calling MCP server methods, which bypasses type checking. This was previously identified in a review.Consider a more type-safe approach:
// Define typed wrapper for prompt registration function registerPrompt( name: string, description: string | undefined, schema: any | undefined, handler: (...args: any[]) => Promise<any> ) { if (description && schema) { this.mcpServer.prompt(name, description, schema, handler); } else if (description) { this.mcpServer.prompt(name, description, handler); } else { this.mcpServer.prompt(name, handler); } }
362-370:⚠️ Potential issuePotential race condition in server initialization.
mcpStreamServerInit,mcpServerInit, andmcpServerRegisterrely onthis.mcpServerbeing available, yet they are called immediately after creation and before awaiting any async work. If later code callsregister()concurrently, there's a potential race condition.Consider awaiting the initialization sequence:
- this.mcpStreamServerInit(); - this.mcpServerInit(); - this.mcpServerRegister(); + await this.mcpStreamServerInit(); + await this.mcpServerInit(); + await this.mcpServerRegister();
🧹 Nitpick comments (4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (4)
87-87: Remove unrelated 'cache-control' header.The
cache-control: no-cacheheader is not directly related to MCP and its SSE implementation. This header seems to be included from typical SSE implementations but not required for MCP functionality.- 'cache-control': 'no-cache',
193-193: Unnecessary 'this' aliasing.Using
const self = thisis unnecessary in arrow functions as they inherit thethiscontext from their enclosing scope.- const self = this;Replace all instances of
selfwiththisin the handler functions that follow.Also applies to: 230-230, 280-280, 308-308, 331-331
🧰 Tools
🪛 Biome (1.9.4)
[error] 193-193: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
208-215: Configurable heartbeat interval not documented.The SSE heartbeat interval is configurable via
app.config.mcp?.sseHeartTimebut there's no documentation about this configuration or its default value (25 seconds).- }, self.app.config.mcp?.sseHeartTime ?? 25000); + // Send SSE keep-alive comments to prevent connection timeout + // Default: 25 seconds; configurable via app.config.mcp.sseHeartTime + }, self.app.config.mcp?.sseHeartTime ?? 25000);
371-379: Improve entry iteration for Maps.The code retrieves controller prototype from the map twice - once during iteration and again when calling the register method.
- for (const [ prompt, controllerProto ] of promptMap.entries()) { - await this.mcpPromptRegister(controllerProto, prompt); - } + for (const [ prompt, controllerProto ] of promptMap) { + await this.mcpPromptRegister(controllerProto, prompt); + }Apply similar changes to the other Map iterations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin/controller/app/middleware/mcp_body_middleware.ts(1 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/app/middleware/mcp_body_middleware.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (11)
core/controller-decorator/src/model/MCPControllerMeta.ts (2)
MCPControllerMeta(7-70)id(22-24)core/controller-decorator/src/model/MCPPromptMeta.ts (1)
MCPPromptMeta(4-30)core/controller-decorator/src/model/MCPToolMeta.ts (1)
MCPToolMeta(5-31)plugin/mcp-proxy/typings/index.d.ts (2)
MCPProtocols(5-5)Application(12-13)plugin/controller/lib/ControllerRegister.ts (1)
ControllerRegister(4-6)core/runtime/src/factory/EggContainerFactory.ts (1)
EggContainerFactory(18-88)core/types/metadata/model/EggPrototype.ts (1)
EggPrototype(117-154)benchmark/http/app/router.js (1)
app(4-4)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (1)
InMemoryEventStore(59-127)core/types/controller-decorator/MetadataKey.ts (1)
CONTROLLER_META_DATA(9-9)core/controller-decorator/src/model/MCPResourceMeta.ts (1)
MCPResourceMeta(5-36)
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 82-82: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 193-193: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 230-230: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 280-280: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 308-308: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 331-331: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (16)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (5)
250-250:⚠️ Potential issueMissing error handling for transport.handlePostMessage.
There's no try/catch block around the transport method call, which could lead to unhandled exceptions if the transport fails.
- await self.transports[sessionId].handlePostMessage(ctx.req, ctx.res); + try { + await self.transports[sessionId].handlePostMessage(ctx.req, ctx.res); + } catch (error) { + self.app.logger.error('Error handling MCP message', error); + if (!ctx.res.headersSent) { + ctx.status = 500; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32603, + message: `Internal error: ${error.message}`, + }, + id: null, + }; + } + }
279-286: 🛠️ Refactor suggestionUse type-safe approach for registering handlers.
The code uses unsafe type casting with
as unknown as [any, any, any, any]when calling MCP server methods, which bypasses type checking.Consider creating properly typed wrappers for the MCP server methods or defining proper interfaces for the arguments to maintain type safety:
- this.mcpServer.prompt(...(args as unknown as [any, any, any, any])); + // Type-safe wrapper for mcpServer.prompt + if (args.length === 4) { + this.mcpServer.prompt(args[0], args[1], args[2], args[3]); + } else if (args.length === 3) { + this.mcpServer.prompt(args[0], args[1], args[2]); + } else if (args.length === 2) { + this.mcpServer.prompt(args[0], args[1]); + } else { + this.mcpServer.prompt(args[0]); + }Same applies to the calls in lines 314 and 337.
105-117:⚠️ Potential issueMissing return after JSON parse error.
The code continues execution after catching a JSON parse error which could lead to undefined behavior. Add a return statement to exit the handler after responding with an error.
try { body = JSON.parse(rawBody); } catch (e) { ctx.status = 400; ctx.body = { jsonrpc: '2.0', error: { code: -32000, message: 'Bad Request: body should is json', }, id: null, }; + return; }
234-234:⚠️ Potential issue
sessionIdis not validated – may crash on undefined.
ctx.query.sessionIdcan beundefined, leading to:
- Using
sessionIdas an object key inself.transports[sessionId]- Could throw "can't convert undefined to string" or access invalid properties
- const sessionId = ctx.query.sessionId; + const sessionId = String(ctx.query.sessionId ?? ''); + if (!sessionId) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32000, + message: 'Bad Request: sessionId is required', + }, + id: null, + }; + return; + }
361-368:⚠️ Potential issueInitialisation order – router hooks may run before
mcpServerready.
mcpStreamServerInit,mcpServerInit, andmcpServerRegisterrely onthis.mcpServerbeing non-null, yet they are called immediately after creation and before awaiting any async work. If later code callsregister()concurrently, there's a race.Wrap the first-time initialisation in a mutex or await the three server-init methods sequentially inside the constructor:
if (!this.mcpServer) { this.mcpServer = new McpServer({ name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, version: this.controllerMeta.version ?? '1.0.0', }, { capabilities: { logging: {} } }); - this.mcpStreamServerInit(); - this.mcpServerInit(); - this.mcpServerRegister(); + // Initialize in sequence to avoid race conditions + await this.mcpStreamServerInit(); + await this.mcpServerInit(); + await this.mcpServerRegister(); }
🧹 Nitpick comments (8)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (8)
29-42: Improve documentation for hook interface methods.The
MCPControllerHookinterface defines several methods but lacks documentation explaining when each method is called and what it should do.Add JSDoc comments to document each hook method:
export interface MCPControllerHook { + /** + * Called before initializing an SSE transport. + * Can be used to customize the transport or perform authentication. + */ // SSE preSSEInitHandle?: (ctx: Context, transport: SSEServerTransport, register: MCPControllerRegister) => Promise<void> + /** + * Called before handling an SSE message request. + * Can be used for authentication or request validation. + */ preHandleInitHandle?: (ctx: Context) => Promise<void> // STREAM + /** + * Called before handling a stream request. + * Can be used for request validation or preprocessing. + */ preHandle?: (ctx: Context) => Promise<void> + /** + * Called when a stream session is initialized. + * Can be used to configure the transport or register session data. + */ onStreamSessionInitialized?: (ctx: Context, transport: StreamableHTTPServerTransport, register: MCPControllerRegister) => Promise<void> // COMMON + /** + * Called before proxying a request to the MCP server. + * Can be used to modify request headers or perform authentication. + */ preProxy?: (ctx: Context, proxyReq: Parameters<typeof http.createServer>['0'], proxyResp: Parameters<typeof http.createServer>['1']) => Promise<void> + /** + * Called to load schemas for tools and prompts. + * Can be used to load schemas from external sources. + */ schemaLoader?: (controllerMeta: MCPControllerMeta, meta: MCPPromptMeta | MCPToolMeta) => Promise<Parameters<McpServer['tool']>['2'] | Parameters<McpServer['prompt']>['2']> + /** + * Called to check if a request should be proxied. + * Return true to indicate the proxy has handled the request. + */ checkAndRunProxy?: (ctx: Context, type: MCPProtocols, sessionId: string) => Promise<boolean>; }
75-78: Document intended usage of hooks API.The static
addHookmethod allows adding hooks to customize the MCP controller behavior, but there's no documentation on when and how to use it.Add documentation comments to explain the intended usage:
+ /** + * Add a hook to customize the behavior of MCP controllers. + * Hooks can modify request/response handling, load schemas, or intercept proxy requests. + * + * @param hook MCPControllerHook implementation with optional lifecycle methods + * @example + * // Register a hook that loads schemas from a JSON file + * MCPControllerRegister.addHook({ + * schemaLoader: async (controllerMeta, meta) => { + * const schema = await loadSchema(`${meta.name}.json`); + * return schema; + * } + * }); + */ static addHook(hook: MCPControllerHook) { MCPControllerRegister.hooks.push(hook); }
82-82: Remove unnecessaryselfvariable.The
selfvariable is used to referencethisinside arrow functions, but arrow functions already inheritthisfrom their enclosing scope.- const self = this;The same issue exists in lines 191, 228, 278, 306, 329.
🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
125-129: Allow configurable event store implementation.The code provides fallback to
InMemoryEventStorebut does not document the configuration option. The event store might need to be persistent in production environments.- const eventStore = this.app.config.mcp?.eventStore ?? new InMemoryEventStore(); + // Use configured event store or fall back to in-memory implementation + // For production, consider configuring a persistent event store implementation + const eventStore = this.app.config.mcp?.eventStore ?? new InMemoryEventStore();
128-128: Make session ID generator configurable.The session ID generator is hardcoded to use either a provided function or
randomUUID(), but there's no documentation about this configuration option.- sessionIdGenerator: () => this.app.config.mcp?.sessionIdGenerator ?? randomUUID(), + // Allow custom session ID generation strategy + sessionIdGenerator: () => { + const generator = this.app.config.mcp?.sessionIdGenerator; + return generator ? generator() : randomUUID(); + },
213-213: Make heartbeat interval configurable.The heartbeat interval is hardcoded to 25 seconds if not configured. This should be documented and possibly have a more descriptive property name.
- }, self.app.config.mcp?.sseHeartTime ?? 25000); + // Send keepalive messages to prevent connection timeout + // Default: 25 seconds (most proxies timeout after 30 seconds of inactivity) + }, self.app.config.mcp?.sseHeartbeatInterval ?? 25000);
349-357: Use Map entries to avoid multiple lookups.The code first puts items in maps and then immediately iterates to call registration methods. This could be more efficient.
- for (const prompt of metadata.prompts) { - promptMap.set(prompt, proto); - } - for (const resource of metadata.resources) { - resourceMap.set(resource, proto); - } - for (const tool of metadata.tools) { - toolMap.set(tool, proto); - } + // Add all metadata items to their respective maps + metadata.prompts.forEach(prompt => promptMap.set(prompt, proto)); + metadata.resources.forEach(resource => resourceMap.set(resource, proto)); + metadata.tools.forEach(tool => toolMap.set(tool, proto));
152-153: Simplify conditional logic in stream handler.The code first checks for the absence of sessionId and then has a redundant check for its presence.
- } else if (sessionId) { + } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (9)
core/controller-decorator/src/model/MCPControllerMeta.ts (2)
MCPControllerMeta(7-70)id(22-24)core/controller-decorator/src/model/MCPPromptMeta.ts (1)
MCPPromptMeta(4-30)core/controller-decorator/src/model/MCPToolMeta.ts (1)
MCPToolMeta(5-31)plugin/controller/lib/ControllerRegister.ts (1)
ControllerRegister(4-6)core/runtime/src/factory/EggContainerFactory.ts (1)
EggContainerFactory(18-88)core/types/metadata/model/EggPrototype.ts (1)
EggPrototype(117-154)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (1)
InMemoryEventStore(59-127)core/types/controller-decorator/MetadataKey.ts (1)
CONTROLLER_META_DATA(9-9)core/controller-decorator/src/model/MCPResourceMeta.ts (1)
MCPResourceMeta(5-36)
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 82-82: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 191-191: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 228-228: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 278-278: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 306-306: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 329-329: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts
Outdated
Show resolved
Hide resolved
plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (15)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (2)
192-195:⚠️ Potential issueAdd proper null checking for
detailbefore using non-null assertionUsing non-null assertion (
!) ondetailin line 192 is risky ifdetailis null or undefined. Adding proper null checking is safer.const detail = await this.app.mcpProxy.getClient(this.ctx.request.query.sessionId); - if (detail?.pid !== process.pid) { + if (!detail) { + this.ctx.status = 404; + this.ctx.body = { + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session not found', + }, + id: null, + }; + return; + } else if (detail.pid !== process.pid) { await this.app.mcpProxy.proxyMessage(this.ctx, { - detail: detail!, + detail, sessionId: this.ctx.request.query.sessionId, type: MCPProtocols.SSE, });
274-279:⚠️ Potential issueAdd proper null checking for
sidbefore using non-null assertionUsing non-null assertion (
!) onsidis risky ifsidis null or undefined. Add proper type checking instead.const sid = transport.sessionId; if (sid && transports[sid]) { delete transports[sid]; + await this.app.mcpProxy.unregisterClient(sid); + } else if (sid) { + await this.app.mcpProxy.unregisterClient(sid); } - await this.app.mcpProxy.unregisterClient(sid!);core/controller-decorator/src/util/MCPInfoUtil.ts (1)
116-124:⚠️ Potential issueFix potential metadata collision between prompts and resources
setMCPPromptParamsandgetMCPPromptParamsare using the resource params map key instead of a dedicated key for prompt parameters. This could lead to metadata collisions where prompt metadata overwrites resource metadata or vice versa.static setMCPPromptParams(params: MCPPromptParams & { mcpName?: string }, clazz: EggProtoImplClass, resourceName: string) { - const methodMap: MCPPromptMap = MetadataUtil.initOwnMapMetaData(CONTROLLER_MCP_RESOURCE_PARAMS_MAP, clazz, new Map()); + const methodMap: MCPPromptMap = MetadataUtil.initOwnMapMetaData(CONTROLLER_MCP_PROMPT_PARAMS_MAP, clazz, new Map()); methodMap.set(resourceName, params); } static getMCPPromptParams(clazz: EggProtoImplClass, resourceName: string): MCPPromptParams & { mcpName?: string } | undefined { - const methodMap: MCPPromptMap | undefined = MetadataUtil.getMetaData(CONTROLLER_MCP_RESOURCE_PARAMS_MAP, clazz); + const methodMap: MCPPromptMap | undefined = MetadataUtil.getMetaData(CONTROLLER_MCP_PROMPT_PARAMS_MAP, clazz); return methodMap?.get(resourceName); }You'll need to define
CONTROLLER_MCP_PROMPT_PARAMS_MAPin your types file if it doesn't exist yet.plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (5)
28-41: Add documentation for hook interface methodsThe
MCPControllerHookinterface defines several methods but lacks documentation explaining when each method is called and what it should do.export interface MCPControllerHook { // SSE + /** + * Called before initializing an SSE transport. + * Can be used to customize the transport or perform authentication. + */ preSSEInitHandle?: (ctx: Context, transport: SSEServerTransport, register: MCPControllerRegister) => Promise<void> + /** + * Called before handling an SSE message request. + * Can be used for authentication or request validation. + */ preHandleInitHandle?: (ctx: Context) => Promise<void> // STREAM + /** + * Called before handling a stream request. + * Can be used for authentication or request validation. + */ preHandle?: (ctx: Context) => Promise<void> + /** + * Called when a stream session is initialized. + * Can be used to customize session behavior or register additional handlers. + */ onStreamSessionInitialized?: (ctx: Context, transport: StreamableHTTPServerTransport, register: MCPControllerRegister) => Promise<void> // COMMON + /** + * Called before proxying a request to another process. + * Can be used to modify the request or response before proxying. + */ preProxy?: (ctx: Context, proxyReq: Parameters<typeof http.createServer>['0'], proxyResp: Parameters<typeof http.createServer>['1']) => Promise<void> + /** + * Called to load schemas for MCP prompts or tools. + * Can be used to customize schema loading from various sources. + */ schemaLoader?: (controllerMeta: MCPControllerMeta, meta: MCPPromptMeta | MCPToolMeta) => Promise<Parameters<McpServer['tool']>['2'] | Parameters<McpServer['prompt']>['2']> + /** + * Called to check if a request should be proxied to another process. + * Return true if the request has been handled by this hook, false otherwise. + */ checkAndRunProxy?: (ctx: Context, type: MCPProtocols, sessionId: string) => Promise<boolean>; }
74-77: Document intended usage of hooks APIThe static
addHookmethod allows adding hooks to customize the MCP controller behavior, but there's no documentation on when and how to use it.+ /** + * Add a hook to customize the behavior of MCP controllers. + * Hooks can modify request/response handling, load schemas, or intercept proxy requests. + * + * @param hook MCPControllerHook implementation with optional lifecycle methods + */ static addHook(hook: MCPControllerHook) { MCPControllerRegister.hooks.push(hook); }
323-326: Use proper type-safe approach for registering handlersThe code uses unsafe type casting with
as unknown as [any, any, any, any]when calling MCP server methods, which bypasses type checking.- this.mcpServer.prompt(...(args as unknown as [any, any, any, any])); + // Create properly typed function signature + type PromptParams = Parameters<typeof this.mcpServer.prompt>; + this.mcpServer.prompt(...(args as PromptParams));Apply similar changes for the other MCP method calls at lines 369 and 392.
415-423: 🛠️ Refactor suggestionImprove initialization order to prevent race conditions
mcpStreamServerInit,mcpServerInit, andmcpServerRegisterrely onthis.mcpServerbeing non-null, yet they are called immediately after creation and before awaiting any async work. If later code callsregister()concurrently, there's a race condition.if (!this.mcpServer) { this.mcpServer = new McpServer({ name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, version: this.controllerMeta.version ?? '1.0.0', }, { capabilities: { logging: {} } }); - this.mcpStreamServerInit(); - this.mcpServerInit(); - this.mcpServerRegister(); + // Ensure sequential initialization to prevent race conditions + await Promise.all([ + this.mcpStreamServerInit(), + this.mcpServerInit(), + this.mcpServerRegister() + ]); }
254-256:⚠️ Potential issueAdd proper sessionId validation in message handler
The message handler does not validate that
sessionIdis defined before using it, which could lead to errors.- const sessionId = ctx.query.sessionId; + const sessionId = String(ctx.query.sessionId ?? ''); + if (!sessionId) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32602, + message: 'Bad Request: No sessionId provided', + }, + id: null, + }; + return; + } if (self.transports[sessionId]) {plugin/mcp-proxy/index.ts (7)
25-25:⚠️ Potential issueFix typo in ProxyAction type and usage
The constant "MCP_SEE_PROXY" should be "MCP_SSE_PROXY" to match the SSE protocol type. This typo appears in both the type definition and usage locations.
-type ProxyAction = 'MCP_STDIO_PROXY' | 'MCP_SEE_PROXY' | 'MCP_STREAM_PROXY'; +type ProxyAction = 'MCP_STDIO_PROXY' | 'MCP_SSE_PROXY' | 'MCP_STREAM_PROXY'; // At line 229: case MCPProtocols.SSE: - action = 'MCP_SEE_PROXY'; + action = 'MCP_SSE_PROXY'; // At line 293: case 'SSE': { - action = 'MCP_SEE_PROXY'; + action = 'MCP_SSE_PROXY';Also applies to: 229-229, 293-293
191-192:⚠️ Potential issueAdd proper null checking for
sidbefore using non-null assertionUsing non-null assertion (
!) onsidis risky ifsidis null or undefined. Add proper type checking instead.const sid = transport.sessionId; if (sid && self.streamTransports[sid]) { delete self.streamTransports[sid]; } - await apiClient.unregisterClient(sid!); + if (sid) { + await apiClient.unregisterClient(sid); + }
196-199:⚠️ Potential issueAdd proper null checking for
detailbefore using non-null assertionUsing non-null assertion (
!) ondetailis risky ifdetailis null or undefined. Add proper type checking before usage.const detail = await apiClient.getClient(sessionId); - if (detail?.pid !== process.pid) { + if (!detail) { + ctx.status = 404; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session not found', + }, + id: null, + }; + return true; + } else if (detail.pid !== process.pid) { await apiClient.proxyMessage(ctx, { - detail: detail!, + detail, sessionId, type, });
216-222: 🛠️ Refactor suggestionAdd error handling for server initialization
The server initialization lacks error handling. If the port is already in use, the promise may never resolve, causing the application to hang.
this.port = this.app.config.mcp?.proxyPort + (cluster.worker?.id ?? 0); this.addProxyHook(); - await new Promise(resolve => server.listen(this.port, () => { - // const address = server.address()! as AddressInfo; - // this.port = address.port; - resolve(null); - })); + await new Promise((resolve, reject) => { + server.once('error', err => { + this.logger.error('Failed to start MCP proxy server:', err); + reject(err); + }); + server.listen(this.port, () => { + this.logger.info(`MCP proxy server listening on port ${this.port}`); + resolve(null); + }); + });
55-55:⚠️ Potential issueInitialize _client field or remove redundant declaration
The subclass redeclares
private _client: any;but never assigns it. The base classAPIClientBasenormally setsthis._clientinternally, but the redeclaration hides that property.- private _client: any;Remove the redundant field declaration to rely on the base-class implementation, or initialize it properly in the
_initmethod.
212-215: 💡 Verification agent🧩 Analysis chain
Enforce validation of
mcp-proxy-typebefore dispatching proxy handlersThe current code casts the untrusted header directly to
ProxyActionand invokes the handler without checking whether it's one of the expected values. This can lead to security vulnerabilities.const server = http.createServer(async (req, res) => { - const type = req.headers['mcp-proxy-type'] as ProxyAction; - await this.proxyHandlerMap[type]?.(req, res); + const typeHeader = req.headers['mcp-proxy-type'] as string; + // Validate that the header matches one of the allowed ProxyAction keys + if (!typeHeader || !this.proxyHandlerMap[typeHeader as ProxyAction]) { + res.writeHead(400).end('Invalid or unsupported MCP proxy type'); + return; + } + const action = typeHeader as ProxyAction; + await this.proxyHandlerMap[action]!(req, res); });Let's verify if the
mcp-proxy-typeheader can be controlled by untrusted sources:
🏁 Script executed:
#!/bin/bash # Find where 'mcp-proxy-type' headers are set rg "mcp-proxy-type" --no-heading -nLength of output: 382
Enforce validation of the
mcp-proxy-typeheader before dispatching proxy handlersThe request handler currently casts the raw header straight to
ProxyActionand invokes the handler, allowing any client-supplied value—even unsupported ones—to be invoked. We should validate that the header matches one of our knownProxyActionkeys and return a 400 if not.• File:
plugin/mcp-proxy/index.ts
• Around lines 212–215, replace:- const type = req.headers['mcp-proxy-type'] as ProxyAction; - await this.proxyHandlerMap[type]?.(req, res);with:
+ const typeHeader = req.headers['mcp-proxy-type'] as string; + // Reject missing or unsupported proxy actions + if (!typeHeader || !this.proxyHandlerMap[typeHeader as ProxyAction]) { + res.writeHead(400).end('Invalid or unsupported MCP proxy type'); + return; + } + const action = typeHeader as ProxyAction; + await this.proxyHandlerMap[action]!(req, res);This ensures only valid, whitelisted proxy actions are executed.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 214-214: Unvalidated dynamic method call
Invocation of method with user-controlled name may dispatch to unexpected target and cause an exception.
294-318: 🛠️ Refactor suggestionRefactor SSE response handling to properly support streaming
For the SSE protocol, the proxy fetches the entire response at once with
resp.text()and sends it back in one chunk. This defeats the purpose of SSE which requires streaming events incrementally.ctx.set(headers); ctx.res.statusCode = resp.status; ctx.res.statusMessage = resp.statusText; - const resData = await resp.text(); - ctx.body = resData; + // Stream the response to preserve SSE functionality + ctx.respond = false; + Readable.fromWeb(resp.body!).pipe(ctx.res);This ensures that SSE events are streamed to the client as they arrive, rather than waiting for all events before sending.
🧹 Nitpick comments (9)
core/controller-decorator/src/model/MCPToolMeta.ts (1)
1-34: Consider adding input validation for critical fieldsThe class structure looks good with readonly properties and proper typing. The constructor properly initializes all fields and coerces
needAclto a boolean.Consider adding validation for critical fields like
nameto prevent potential runtime issues:constructor(opt: { name: string; middlewares: MiddlewareFunc[]; needAcl?: boolean; aclCode?: string, description?: string; mcpName?: string; detail?: ToolArgsSchemaDetail; extra?: number; }) { + if (!opt.name || opt.name.trim() === '') { + throw new Error('MCPToolMeta name cannot be empty'); + } + if (!Array.isArray(opt.middlewares)) { + throw new Error('MCPToolMeta middlewares must be an array'); + } this.name = opt.name; this.needAcl = !!opt.needAcl; this.description = opt.description; this.mcpName = opt.mcpName; this.middlewares = opt.middlewares; this.aclCode = opt.aclCode; this.detail = opt.detail; this.extra = opt.extra; }core/controller-decorator/src/decorator/mcp/MCPPrompt.ts (2)
10-33: Consider explicit error handling for decorator applicationThe
MCPPromptdecorator correctly configures the method for MCP use, but lacks error handling if applied incorrectly.Consider adding error handling to detect incorrect usage and provide better diagnostic messages:
export function MCPPrompt(params?: MCPPromptParams) { return function( target: any, propertyKey: PropertyKey, ) { + if (typeof propertyKey !== 'string') { + throw new TypeError('MCPPrompt decorator can only be applied to methods with string names'); + } const controllerClazz = target.constructor as EggProtoImplClass; const methodName = propertyKey as string; MethodInfoUtil.setMethodControllerType( controllerClazz, methodName, ControllerType.MCP, ); MCPInfoUtil.setMCPPromptParams( { ...params, mcpName: params?.name, name: methodName, }, controllerClazz, methodName, ); MCPInfoUtil.setMCPPrompt(controllerClazz, methodName); }; }
35-52: Consider validation for argsSchema parameterThe
PromptArgsSchemadecorator correctly records schema information but doesn't validate the schema parameter.Add basic validation to ensure the schema parameter is correctly provided:
export function PromptArgsSchema(argsSchema: Parameters<McpServer['prompt']>['2']) { return function( target: any, propertyKey: PropertyKey, parameterIndex: number, ) { + if (!argsSchema) { + throw new TypeError('PromptArgsSchema requires a valid schema parameter'); + } const controllerClazz = target.constructor as EggProtoImplClass; const methodName = propertyKey as string; MCPInfoUtil.setMCPPromptArgsInArgs( { argsSchema, index: parameterIndex, }, controllerClazz, methodName, ); }; }plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (4)
171-172: Remove unnecessary this aliasingArrow functions inherit
thisfrom their enclosing scope, making this aliasing unnecessary.- const self = this; - const transport = new SSEServerTransport('/message', this.ctx.res); + const transport = new SSEServerTransport('/message', this.ctx.res);🧰 Tools
🪛 Biome (1.9.4)
[error] 171-171: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
174-176: Simplify and improve the proxy handler registrationThe handler registration uses
selfunnecessarily and could be simplified.- this.app.mcpProxy.setProxyHandler(MCPProtocols.SSE, async (req, res) => { - return await self.ssePostHandler(req, res); - }); + this.app.mcpProxy.setProxyHandler(MCPProtocols.SSE, (req, res) => + this.ssePostHandler(req, res) + );
261-262: Remove unnecessary this aliasingArrow functions inherit
thisfrom their enclosing scope, making this aliasing unnecessary.- const eventStore = new InMemoryEventStore(); - const self = this; + const eventStore = new InMemoryEventStore();🧰 Tools
🪛 Biome (1.9.4)
[error] 261-261: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
267-270: Use a consistent approach for proxy handler registrationThe handler registration could be simplified for better readability.
- this.app.mcpProxy.setProxyHandler(MCPProtocols.STREAM, async (req, res) => { - return await self.streamPostHandler(req, res); - }); + this.app.mcpProxy.setProxyHandler(MCPProtocols.STREAM, (req, res) => + this.streamPostHandler(req, res) + );core/controller-decorator/src/util/MCPInfoUtil.ts (1)
39-155: Consider using namespace or functions instead of a static-only classThis class contains only static methods and no instance properties. Consider using a namespace or individual functions to improve maintainability.
-export class MCPInfoUtil { +export namespace MCPInfoUtil { // All methods stay the same, just remove the 'static' keywordAlternatively, you could export individual functions directly:
export function setMCPName(name: string, clazz: EggProtoImplClass) { MetadataUtil.defineMetaData(CONTROLLER_MCP_NAME, name, clazz); } export function getMCPName(clazz: EggProtoImplClass): string | undefined { return MetadataUtil.getMetaData(CONTROLLER_MCP_NAME, clazz); } // ... other functions🧰 Tools
🪛 Biome (1.9.4)
[error] 39-155: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
81-81: Remove unnecessary this aliasingArrow functions inherit
thisfrom their enclosing scope, making this aliasing unnecessary in multiple places.- const self = this;Make this change at lines 81, 117, 195, 249, 303, 347, and 384.
Also applies to: 117-117, 195-195, 249-249, 303-303, 347-347, 384-384
🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
core/controller-decorator/index.ts(2 hunks)core/controller-decorator/src/decorator/mcp/Extra.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPPrompt.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPTool.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerResourceMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerToolMetaBuilder.ts(1 hunks)core/controller-decorator/src/model/MCPPromptMeta.ts(1 hunks)core/controller-decorator/src/model/MCPResourceMeta.ts(1 hunks)core/controller-decorator/src/model/MCPToolMeta.ts(1 hunks)core/controller-decorator/src/util/MCPInfoUtil.ts(1 hunks)core/controller-decorator/test/MCPMeta.test.ts(1 hunks)core/controller-decorator/test/fixtures/MCPController.ts(1 hunks)core/types/controller-decorator/MCPPromptParams.ts(1 hunks)core/types/controller-decorator/MCPToolParams.ts(1 hunks)core/types/controller-decorator/MetadataKey.ts(1 hunks)plugin/controller/app.ts(2 hunks)plugin/controller/app/middleware/mcp_body_middleware.ts(1 hunks)plugin/controller/config/config.default.ts(1 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts(1 hunks)plugin/mcp-proxy/config/config.default.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)plugin/mcp-proxy/package.json(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- plugin/mcp-proxy/config/config.default.ts
- core/controller-decorator/src/decorator/mcp/Extra.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- plugin/controller/app/middleware/mcp_body_middleware.ts
- plugin/controller/app.ts
- core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts
- core/controller-decorator/index.ts
- core/controller-decorator/test/MCPMeta.test.ts
- core/controller-decorator/src/impl/mcp/MCPControllerResourceMetaBuilder.ts
- core/controller-decorator/src/model/MCPResourceMeta.ts
- core/controller-decorator/src/impl/mcp/MCPControllerToolMetaBuilder.ts
- core/controller-decorator/src/model/MCPPromptMeta.ts
- core/controller-decorator/src/decorator/mcp/MCPTool.ts
- plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts
- plugin/mcp-proxy/package.json
- core/types/controller-decorator/MCPToolParams.ts
- plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts
- core/types/controller-decorator/MCPPromptParams.ts
- core/controller-decorator/test/fixtures/MCPController.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
plugin/controller/config/config.default.ts (3)
plugin/mcp-proxy/config/config.default.ts (1)
config(1-10)plugin/controller/test/fixtures/apps/mcp-app/config/config.default.js (1)
config(4-11)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/config.default.js (1)
config(4-14)
core/controller-decorator/src/model/MCPToolMeta.ts (2)
core/controller-decorator/src/util/MCPInfoUtil.ts (1)
ToolArgsSchemaDetail(25-28)core/types/controller-decorator/model/types.ts (1)
MiddlewareFunc(5-5)
plugin/mcp-proxy/index.ts (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
MCPControllerHook(28-41)MCPControllerRegister(44-434)plugin/mcp-proxy/lib/MCPProxyDataClient.ts (1)
MCPProxyDataClient(4-39)
🪛 Biome (1.9.4)
core/controller-decorator/src/util/MCPInfoUtil.ts
[error] 39-155: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 195-195: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 249-249: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 303-303: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 386-386: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts
[error] 171-171: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 261-261: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
🪛 GitHub Check: CodeQL
plugin/mcp-proxy/index.ts
[failure] 214-214: Unvalidated dynamic method call
Invocation of method with user-controlled name may dispatch to unexpected target and cause an exception.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (18)
🔇 Additional comments (2)
plugin/controller/config/config.default.ts (1)
1-15:Details
✅ Verification successful
Ensure configuration respects user overrides and existing CSRF ignore settings
This new default configuration for MCP paths looks good, but based on previous reviews, you need to:
- Make sure these paths respect user configuration rather than being hard-coded
- Ensure compatibility with existing CSRF ignore settings when these paths are added to the ignore list
Consider updating the implementation to follow the pattern used in other Egg.js configurations where user settings take precedence and are properly merged with defaults.
🏁 Script executed:
#!/bin/bash # Check how other configs handle merging with user settings grep -r "security.csrf.ignore" --include="*.ts" .Length of output: 635
🏁 Script executed:
#!/bin/bash # Show how CSRF ignore paths are merged in plugin/controller/app.ts grep -R -n -A5 -B5 "security.csrf.ignore" plugin/controller/app.tsLength of output: 1115
No changes needed—CSRF ignore merging already respects user overrides
Theplugin/controller/app.tslogic prepends the MCP paths toapp.config.security.csrf.ignorewhen it exists (and initializes it otherwise), so any user-defined ignore entries are preserved. The default MCP paths inconfig.default.tscan still be overridden via the standard Egg.js config merge.core/types/controller-decorator/MetadataKey.ts (1)
27-37: Clear and consistent Symbol naming for MCP metadata keysThe new metadata symbols for MCP follow the established pattern and naming conventions of the codebase, providing clear intent for each key.
The naming is consistent and clearly describes the purpose of each metadata key, making them easy to understand and use throughout the MCP implementation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
plugin/mcp-proxy/index.ts (2)
55-70:⚠️ Potential issue
_clientshadowing breaks every proxy API call.
APIClientBasealready declares a protected_client; redeclaring it here leaves the new field undefined, so
this._client.registerClient(...)crashes at runtime.- private _client: any;Delete the field and rely on the base-class implementation.
321-333: 🛠️ Refactor suggestionSSE proxy response is buffered, destroying stream semantics.
await resp.text()loads the entire stream into memory and prevents incremental delivery.
Pipe the body instead:-const resData = await resp.text(); -ctx.body = resData; +ctx.respond = false; +Readable.fromWeb(resp.body!).pipe(ctx.res);This matches SSE’s event streaming model and avoids unbounded memory usage.
🧹 Nitpick comments (3)
core/controller-decorator/src/util/MCPInfoUtil.ts (2)
94-102: Parameter name is misleading (resourceNameused for tools).
setMCPToolParams/getMCPToolParamsaccept aresourceNameargument, although these APIs clearly deal with tools.
This confuses readers and increases the chance of copy-paste errors.-static getMCPToolParams(clazz: EggProtoImplClass, resourceName: string): ... +static getMCPToolParams(clazz: EggProtoImplClass, toolName: string): ... ... -static setMCPToolParams(params, clazz, resourceName: string) { - const methodMap = ... - methodMap.set(resourceName, params); +static setMCPToolParams(params, clazz, toolName: string) { + const methodMap = ... + methodMap.set(toolName, params); }
40-57: Class contains only statics – consider a namespace or plain functions.Biome warns about “noStaticOnlyClass”.
Using a namespace / module with exported helper functions avoids the unnecessary class wrapper and the need for theexport class MCPInfoUtilconstruct, resulting in tree-shakable code with zero runtime cost.Not blocking, but worth considering for a follow-up refactor.
🧰 Tools
🪛 Biome (1.9.4)
[error] 40-156: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
80-88: Unnecessaryself = thisaliasing clutters code.Inside arrow functions
thisis lexically bound; storing it inselfis redundant and violates the project’s clean-code guidelines surfaced by Biome.Search & drop the pattern across the file:
-const self = this; -const initHandler = async (ctx) => { - ... - if (MCPControllerRegister.hooks.length > 0) { - for (const hook of MCPControllerRegister.hooks) { - await hook.preHandle?.(self.app.currentContext); +const initHandler = async (ctx) => { + ... + for (const hook of MCPControllerRegister.hooks) { + await hook.preHandle?.(this.app.currentContext); }🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/controller-decorator/src/util/MCPInfoUtil.ts(1 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)plugin/mcp-proxy/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/mcp-proxy/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 196-196: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 250-250: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 319-319: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 365-365: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 402-402: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
core/controller-decorator/src/util/MCPInfoUtil.ts
[error] 40-156: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (20)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (7)
28-41: Improve documentation for hook interface methodsThe
MCPControllerHookinterface defines several important methods but lacks documentation explaining when each method is called and what it should do.Add JSDoc comments to document each hook method:
export interface MCPControllerHook { + /** + * Called before initializing an SSE transport. + * Can be used to customize the transport or perform authentication. + */ preSSEInitHandle?: (ctx: Context, transport: SSEServerTransport, register: MCPControllerRegister) => Promise<void> + /** + * Called before handling an SSE message request. + * Can be used for authentication or request validation. + */ preHandleInitHandle?: (ctx: Context) => Promise<void> // SSE + /** + * Called before handling a stream request. + * Can be used for authentication or request validation. + */ preHandle?: (ctx: Context) => Promise<void> + /** + * Called when a stream session is initialized. + * Can be used to set up session-specific resources or handlers. + */ onStreamSessionInitialized?: (ctx: Context, transport: StreamableHTTPServerTransport, register: MCPControllerRegister) => Promise<void>
74-77: Document intended usage of hooks APIThe static
addHookmethod allows adding hooks to customize the MCP controller behavior, but there's no documentation on when and how to use it.Add documentation comments to explain the intended usage:
+ /** + * Add a hook to customize the behavior of MCP controllers. + * Hooks can modify request/response handling, load schemas, or intercept proxy requests. + * + * @param hook MCPControllerHook implementation with optional lifecycle methods + */ static addHook(hook: MCPControllerHook) { MCPControllerRegister.hooks.push(hook); }
255-282: Validate sessionId before using itThe messageHandler doesn't validate
sessionIdbefore using it to access the transports object, which could lead to errors ifsessionIdis undefined.Add explicit validation before proceeding:
- const sessionId = ctx.query.sessionId; + const sessionId = String(ctx.query.sessionId ?? ''); + if (!sessionId) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32000, + message: 'Bad Request: sessionId required', + }, + id: null, + }; + return; + }
117-117: Document event store configuration optionThe code provides a fallback to
InMemoryEventStorebut does not document the configuration option.Add a comment documenting the configuration option:
- const eventStore = this.app.config.mcp?.eventStore ?? new InMemoryEventStore(); + // Use configured event store or fall back to in-memory implementation + // For production, consider configuring a persistent event store implementation + const eventStore = this.app.config.mcp?.eventStore ?? new InMemoryEventStore();
432-439: 🛠️ Refactor suggestionPotential race condition in server initialization
The initialization of
mcpServerand subsequent calls happen without any synchronization mechanism, which could lead to race conditions ifregister()is called concurrently.Wrap the first-time initialization in a way that prevents race conditions:
- if (!this.mcpServer) { - this.mcpServer = new McpServer({ - name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, - version: this.controllerMeta.version ?? '1.0.0', - }, { capabilities: { logging: {} } }); - this.mcpStreamServerInit(); - this.mcpServerInit(); - this.mcpServerRegister(); - } + if (!this.mcpServer) { + // Create the server only once + this.mcpServer = new McpServer({ + name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, + version: this.controllerMeta.version ?? '1.0.0', + }, { capabilities: { logging: {} } }); + + // Initialize in sequence to avoid race conditions + await Promise.resolve() + .then(() => this.mcpStreamServerInit()) + .then(() => this.mcpServerInit()) + .then(() => this.mcpServerRegister()); + }
199-199:⚠️ Potential issueAdd optional chaining for config value access
The code accesses
self.app.config.mcp.sseMessagePathwithout optional chaining, which could lead to runtime errors if the configuration is missing.Add optional chaining and provide a fallback:
- const transport = new SSEServerTransport(self.app.config.mcp.sseMessagePath, ctx.res); + const transport = new SSEServerTransport( + self.app.config.mcp?.sseMessagePath ?? '/api/mcp/message', + ctx.res + );
300-342:⚠️ Potential issueFix bug in mcpPromptRegister arguments handling
There's a critical bug in the
mcpPromptRegistermethod -newArgsis correctly built but the originalargsare passed to the method instead.The method builds
newArgsto align arguments with the controller method's signature but then passes the originalargstoReflect.apply, discarding the transformation. This will break prompts that use@MCPPromptArgsor@MCPExtradecorators.let newArgs: any[] = []; if (schema && promptMeta.detail) { // 如果有 schema 则证明入参第一个就是 schema newArgs[promptMeta.detail.index] = args[0]; // 如果有 schema 则证明入参第二个就是 extra if (promptMeta.extra) { newArgs[promptMeta.extra] = args[1]; } } else if (promptMeta.extra) { // 无 schema, 那么入参第一个就是 extra newArgs[promptMeta.extra] = args[0]; } - newArgs = [ ...newArgs, ...args ]; + // Fill any gaps in newArgs with undefined + const maxIndex = Math.max( + promptMeta.detail?.index ?? -1, + promptMeta.extra ?? -1 + ); + for (let i = 0; i <= maxIndex; i++) { + if (newArgs[i] === undefined) { + newArgs[i] = undefined; + } + } - return Reflect.apply(realMethod, realObj, newArgs); + return Reflect.apply(realMethod, realObj, newArgs);Also, translate the Chinese comments for better maintainability:
- // 如果有 schema 则证明入参第一个就是 schema + // If there's a schema, the first argument is the schema input - // 如果有 schema 则证明入参第二个就是 extra + // If there's a schema, the second argument is the extra - // 无 schema, 那么入参第一个就是 extra + // Without schema, the first argument is the extra🧰 Tools
🪛 Biome (1.9.4)
[error] 319-319: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
🧹 Nitpick comments (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (3)
341-341: Replace unsafe type casting with proper type definitionThe code uses unsafe type casting with
as unknown as [any, any, any, any]when calling MCP server methods, which bypasses type checking.Consider using more type-safe approaches:
- this.mcpServer.prompt(...(args as unknown as [any, any, any, any])); + type PromptArgs = Parameters<typeof this.mcpServer.prompt>; + this.mcpServer.prompt(...(args as unknown as PromptArgs));Or define a properly typed wrapper method:
private typedPrompt(name: string, description?: string, schema?: any, handler?: any): void { this.mcpServer.prompt(name, description, schema, handler); }Then use this method instead of direct calls with type casting.
81-81: Remove unnecessary this aliasing in arrow functionsSeveral places in the code use unnecessary
thisaliasing withconst self = this. Arrow functions inheritthisfrom their enclosing scope, making this alias redundant.Remove the unnecessary alias:
- const self = this;And replace all occurrences of
selfwiththisin each of the arrow functions where this pattern is used.Also applies to: 196-196, 250-250, 319-319, 365-365, 402-402
🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
307-318: Add error handling for schema loading hooksThe code runs schema loader hooks without try/catch blocks, which could lead to unhandled promise rejections if a hook throws an error.
Add error handling for schema loading:
if (promptMeta.detail?.argsSchema) { schema = promptMeta.detail?.argsSchema; args.push(schema); } else if (MCPControllerRegister.hooks.length > 0) { for (const hook of MCPControllerRegister.hooks) { - schema = await hook.schemaLoader?.(controllerMeta, promptMeta); + try { + schema = await hook.schemaLoader?.(controllerMeta, promptMeta); + } catch (error) { + this.app.logger.error('Error loading schema for prompt %s: %o', promptMeta.name, error); + // Continue with other hooks but log the error + continue; + } if (schema) { args.push(schema); break; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (8)
core/controller-decorator/src/model/MCPControllerMeta.ts (2)
MCPControllerMeta(7-70)id(22-24)core/controller-decorator/src/model/MCPPromptMeta.ts (1)
MCPPromptMeta(4-33)core/controller-decorator/src/model/MCPToolMeta.ts (1)
MCPToolMeta(5-34)plugin/controller/lib/ControllerRegister.ts (1)
ControllerRegister(4-6)core/runtime/src/factory/EggContainerFactory.ts (1)
EggContainerFactory(18-88)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (1)
InMemoryEventStore(59-127)core/types/controller-decorator/MetadataKey.ts (1)
CONTROLLER_META_DATA(9-9)core/controller-decorator/src/model/MCPResourceMeta.ts (1)
MCPResourceMeta(5-39)
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 196-196: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 250-250: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 319-319: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 365-365: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 402-402: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (14)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (5)
337-339:⚠️ Potential issueFix critical bug in
mcpPromptRegisterarguments handling.The function builds
newArgswith correct ordering based on metadata but then uses the originalargsin theReflect.applycall, making the reordering ineffective.- newArgs = [ ...newArgs, ...args ]; - return Reflect.apply(realMethod, realObj, args); + // Fill any gaps in newArgs with undefined + const maxIndex = Math.max( + promptMeta.detail?.index ?? -1, + promptMeta.extra ?? -1 + ); + for (let i = 0; i <= maxIndex; i++) { + if (newArgs[i] === undefined) { + newArgs[i] = undefined; + } + } + return Reflect.apply(realMethod, realObj, newArgs);
255-258:⚠️ Potential issueValidate
sessionIdfrom query parameters.
sessionIdfrom query parameters is used without validation, which could lead to errors when accessingself.transports[sessionId].- const sessionId = ctx.query.sessionId; + const sessionId = ctx.query.sessionId as string | undefined; + + if (!sessionId) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32000, + message: 'Bad Request: sessionId is required', + }, + id: null, + }; + return; + } if (self.transports[sessionId]) { // ...
432-439: 🛠️ Refactor suggestionAdd sequential initialization to prevent race conditions.
The
mcpStreamServerInit(),mcpServerInit(), andmcpServerRegister()methods are called immediately after creating the server but before awaiting any async work. This could lead to race conditions ifregister()is called concurrently.if (!this.mcpServer) { this.mcpServer = new McpServer({ name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, version: this.controllerMeta.version ?? '1.0.0', }, { capabilities: { logging: {} } }); - this.mcpStreamServerInit(); - this.mcpServerInit(); - this.mcpServerRegister(); + // Initialize servers sequentially to prevent race conditions + await this.mcpStreamServerInit(); + await this.mcpServerInit(); + await this.mcpServerRegister(); }
369-383:⚠️ Potential issueFix the same argument handling bug in
mcpToolRegister.The same bug exists in the
mcpToolRegistermethod -newArgsis created but then not used in the finalReflect.applycall.let newArgs: any[] = []; if (schema && toolMeta.detail) { // If there's a schema, the first parameter is the schema newArgs[toolMeta.detail.index] = args[0]; // If there's a schema, the second parameter is extra if (toolMeta.extra) { newArgs[toolMeta.extra] = args[1]; } } else if (toolMeta.extra) { // Without schema, the first parameter is extra newArgs[toolMeta.extra] = args[0]; } - newArgs = [ ...newArgs, ...args ]; + // Fill any gaps in newArgs with undefined + const maxIndex = Math.max( + toolMeta.detail?.index ?? -1, + toolMeta.extra ?? -1 + ); + for (let i = 0; i <= maxIndex; i++) { + if (newArgs[i] === undefined) { + newArgs[i] = undefined; + } + } - return Reflect.apply(realMethod, realObj, args); + return Reflect.apply(realMethod, realObj, newArgs);
83-180: 🛠️ Refactor suggestionFix the session validation and response flow in
mcpStreamServerInit.Setting
ctx.respond = falsetoo early prevents proper error responses. The method also lacks proper error handling when a session ID is provided but not found.- ctx.respond = false; if (MCPControllerRegister.hooks.length > 0) { // Hooks execution... } const sessionId = ctx.req.headers['mcp-session-id'] as string | undefined; // ...existing code for when sessionId is not provided... } else if (sessionId) { const transport = self.streamTransports[sessionId]; if (transport) { // ...existing code... + ctx.respond = false; // ...rest of the code... } if (MCPControllerRegister.hooks.length > 0) { // ...hooks execution... } + // If we get here, no transport was found and no hook handled it + ctx.status = 404; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session not found', + }, + id: null, + }; + return; } - return;plugin/mcp-proxy/index.ts (9)
25-25:⚠️ Potential issueFix typo in ProxyAction type and usage ("SEE" should be "SSE").
The constant "MCP_SEE_PROXY" should be "MCP_SSE_PROXY" to match the SSE protocol type. This typo appears in the type definition and usage locations.
-type ProxyAction = 'MCP_STDIO_PROXY' | 'MCP_SEE_PROXY' | 'MCP_STREAM_PROXY'; +type ProxyAction = 'MCP_STDIO_PROXY' | 'MCP_SSE_PROXY' | 'MCP_STREAM_PROXY';Also apply to lines 247 and 311 where this type is used.
54-56:⚠️ Potential issueRemove redundant
_clientfield declaration.The class extends
APIClientBasewhich already has a_clientproperty. Declaring it again hides the property from the base class.export class MCPProxyApiClient extends APIClientBase { - private _client: any; private logger: EggLogger; private proxyHandlerMap: { [P in ProxyAction]?: StreamableHTTPServerTransport['handleRequest']; } = {};
209-210: 🛠️ Refactor suggestionAdd proper null checking for
sidbefore using non-null assertion.Using non-null assertion (
!) onsidis risky ifsidis null or undefined. Add proper type checking instead.const sid = transport.sessionId; if (sid && self.streamTransports[sid]) { delete self.streamTransports[sid]; } - await apiClient.unregisterClient(sid!); + if (sid) { + await apiClient.unregisterClient(sid); + }
246-254:⚠️ Potential issueFix the SSE protocol action mapping.
The SSE protocol is mapped to 'MCP_SEE_PROXY' which appears to be a typo and should be 'MCP_SSE_PROXY'.
switch (type) { case MCPProtocols.SSE: - action = 'MCP_SEE_PROXY'; + action = 'MCP_SSE_PROXY'; break; case MCPProtocols.STDIO: action = 'MCP_STDIO_PROXY'; break; default: action = 'MCP_STREAM_PROXY'; break; }
94-103:⚠️ Potential issueFix ServerResponse method usage.
Using
res.status(400).json()on a Node.jsServerResponseobject will throw at runtime because these methods don't exist on that object.- res.status(400).json({ + res.writeHead(400, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ jsonrpc: '2.0', error: { code: -32000, message: 'Bad Request: Session exists but uses a different transport protocol', }, id: null, -}); +}));Apply this pattern to all similar response patterns in the file.
310-311:⚠️ Potential issueFix the SSE protocol action mapping in the proxy message handler.
The same typo appears here, mapping SSE to 'MCP_SEE_PROXY'.
switch (type) { case 'SSE': { - action = 'MCP_SEE_PROXY'; + action = 'MCP_SSE_PROXY'; ctx.req.headers['mcp-proxy-type'] = action;
122-130:⚠️ Potential issueFix ServerResponse method usage here too.
Same issue with using Express-style methods on a Node.js
ServerResponse.- res.status(404).json({ + res.writeHead(404, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ jsonrpc: '2.0', error: { code: -32602, message: 'Bad Request: No transport found for sessionId', }, id: null, -}); +}));
230-233:⚠️ Potential issueValidate the
mcp-proxy-typeheader before using it.The code uses the header value directly to index into a handler map without validation, which is a security risk.
- const type = req.headers['mcp-proxy-type'] as ProxyAction; - await this.proxyHandlerMap[type]?.(req, res); + const typeHeader = req.headers['mcp-proxy-type'] as string; + // Validate that the header matches one of the allowed ProxyAction keys + if (!typeHeader || !this.proxyHandlerMap[typeHeader as ProxyAction]) { + res.writeHead(400, { 'content-type': 'application/json' }).end(JSON.stringify({ + jsonrpc: '2.0', + error: { + code: -32000, + message: 'Invalid or unsupported MCP proxy type', + }, + id: null, + })); + return; + } + const action = typeHeader as ProxyAction; + await this.proxyHandlerMap[action]!(req, res);
214-220: 🛠️ Refactor suggestionAdd proper null checking for
detailbefore using non-null assertion.Using non-null assertion (
!) ondetailis risky ifdetailis null or undefined. Add proper type checking before usage.const detail = await apiClient.getClient(sessionId); - if (detail?.pid !== process.pid) { + if (!detail) { + ctx.status = 404; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session not found', + }, + id: null, + }; + return true; + } else if (detail.pid !== process.pid) { await apiClient.proxyMessage(ctx, { - detail: detail!, + detail, sessionId, type, });
🧹 Nitpick comments (7)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (5)
28-41: Document the hook interface methods.The
MCPControllerHookinterface defines several important lifecycle hooks without any documentation. Add JSDoc comments to explain when each hook is called and what it should do.export interface MCPControllerHook { + /** + * Called before initializing an SSE transport. Useful for authentication, + * authorization, or custom transport configuration. + */ preSSEInitHandle?: (ctx: Context, transport: SSEServerTransport, register: MCPControllerRegister) => Promise<void> + /** + * Called before handling an SSE message request. + */ preHandleInitHandle?: (ctx: Context) => Promise<void> // STREAM + /** + * Called before handling a STREAM request. + */ preHandle?: (ctx: Context) => Promise<void> + /** + * Called when a STREAM transport session is initialized. + */ onStreamSessionInitialized?: (ctx: Context, transport: StreamableHTTPServerTransport, register: MCPControllerRegister) => Promise<void> // COMMON + /** + * Called before proxying a request to another process or server. + */ preProxy?: (ctx: Context, proxyReq: http.IncomingMessage, proxyResp: http.ServerResponse) => Promise<void> + /** + * Called to load JSON schema for MCP prompt or tool. + */ schemaLoader?: (controllerMeta: MCPControllerMeta, meta: MCPPromptMeta | MCPToolMeta) => Promise<Parameters<McpServer['tool']>['2'] | Parameters<McpServer['prompt']>['2']> + /** + * Called to check if the request should be proxied to another process. + * Return true if the proxy was handled, false otherwise. + */ checkAndRunProxy?: (ctx: Context, type: MCPProtocols, sessionId: string) => Promise<boolean>; }
74-77: Document intended usage of the hooks API.The static
addHookmethod lacks documentation explaining how and when to use it, which is crucial for developers integrating with this API.+ /** + * Add a hook to customize the behavior of MCP controllers. + * Hooks can intercept and modify requests, responses, and other lifecycle events. + * Multiple hooks can be registered and will be executed in the order they were added. + * + * @param hook The hook implementation with optional lifecycle methods + */ static addHook(hook: MCPControllerHook) { MCPControllerRegister.hooks.push(hook); }
81-81: Remove unnecessarythisaliases.Arrow functions inherit
thisfrom their enclosing scope, making these aliases unnecessary and potentially confusing.- const self = this;This applies to lines 81, 196, 250, 319, 364, and 401.
Also applies to: 196-196, 250-250, 319-319, 364-365, 401-402
🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
326-338: Translate the Chinese comments for better maintainability.The file has Chinese comments which can be difficult for international contributors to understand.
- // 如果有 schema 则证明入参第一个就是 schema + // If there's a schema, the first parameter is the schema newArgs[promptMeta.detail.index] = args[0]; - // 如果有 schema 则证明入参第二个就是 extra + // If there's a schema, the second parameter is extra if (promptMeta.extra) { newArgs[promptMeta.extra] = args[1]; } } else if (promptMeta.extra) { - // 无 schema, 那么入参第一个就是 extra + // Without schema, the first parameter is extra newArgs[promptMeta.extra] = args[0]; }
341-342: Use type-safe approach for registering handlers.The code uses unsafe type casting with
as unknown as [any, any, any, any]when calling MCP server methods, which bypasses type checking.- this.mcpServer.prompt(...(args as unknown as [any, any, any, any])); + // Define specific generic types for each handler to improve type safety + type PromptHandler = Parameters<typeof this.mcpServer.prompt>; + this.mcpServer.prompt(...(args as unknown as PromptHandler));Apply similar changes to the other MCP server method calls.
Also applies to: 385-386, 408-409
plugin/mcp-proxy/index.ts (2)
73-74: Remove unnecessarythisalias with ESLint disable comment.The ESLint disable comment is used but you're still using a
thisalias, which is unnecessary in arrow functions.- // eslint-disable-next-line @typescript-eslint/no-this-alias - const apiClient = this;Update the code to use
thisdirectly instead ofapiClientthroughout the hooks.
274-304: Enhance error handling for getRawBody failures.The error handling for request body parsing has room for improvement. The current code catches errors but doesn't specify what types are expected.
try { let encoding = 'utf-8'; if (ctx.req.headers['content-type']) { const ct = contentType.parse(ctx.req.headers['content-type'] ?? ''); if (ct.type !== 'application/json') { throw new Error(`Unsupported content-type: ${ct}`); } encoding = ct.parameters.charset; } // ctx.respond = false; body = await getRawBody(ctx.req, { limit: MAXIMUM_MESSAGE_SIZE, encoding, }); - } catch (error) { + } catch (error: unknown) { this.logger.error(error); + const errorMessage = error instanceof Error ? error.message : String(error); ctx.res.writeHead(400).end(JSON.stringify({ jsonrpc: '2.0', error: { code: -32602, - message: `Bad Request: ${String(error)}`, + message: `Bad Request: ${errorMessage}`, }, id: null, })); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/mcp-proxy/index.ts (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
MCPControllerHook(28-41)MCPControllerRegister(44-450)plugin/mcp-proxy/lib/MCPProxyDataClient.ts (1)
MCPProxyDataClient(4-39)
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 196-196: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 250-250: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 319-319: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 365-365: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 402-402: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (22)
core/controller-decorator/src/decorator/mcp/MCPController.ts (4)
8-8: Rename shadowed parameter to avoidnoShadowRestrictedNameslint errorUsing
constructoras the parameter name shadows the globalconstructorproperty and fails@biome/ ESLint restricted-names rules. Renaming also makes the intent (the decorated class) clearer.- return function(constructor: EggProtoImplClass) { + return function(clazz: EggProtoImplClass) {🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
26-31:⚠️ Potential issueUpdate the remaining instances of the renamed parameter
Complete the renaming in the MCP metadata setting.
if (param?.name) { - MCPInfoUtil.setMCPName(param.name, constructor); + MCPInfoUtil.setMCPName(param.name, clazz); } if (param?.version) { - MCPInfoUtil.setMCPVersion(param.version, constructor); + MCPInfoUtil.setMCPVersion(param.version, clazz); }
24-24:⚠️ Potential issueUse the renamed parameter in the file path registration
Apply the parameter rename consistently in the
setFilePathcall as well.- PrototypeUtil.setFilePath(constructor, StackUtil.getCalleeFromStack(false, 5)); + PrototypeUtil.setFilePath(clazz, StackUtil.getCalleeFromStack(false, 5));
13-17:⚠️ Potential issueUpdate all occurrences of the renamed parameter
After renaming the parameter in the function signature, make sure to update all references to it within the function body.
- func(constructor); - ControllerInfoUtil.setControllerType(constructor, ControllerType.MCP); - if (param?.controllerName) { - ControllerInfoUtil.setControllerName(constructor, param?.controllerName); - } + func(clazz); + ControllerInfoUtil.setControllerType(clazz, ControllerType.MCP); + if (param?.controllerName) { + ControllerInfoUtil.setControllerName(clazz, param?.controllerName); + }plugin/mcp-proxy/test/proxy.test.ts (3)
48-48: Add proper null checking instead of using non-null assertionUsing the non-null assertion operator (
!) can lead to runtime errors ifitem.datais actually null. Consider using proper null checking or providing a default value.- text: item.data!.toString(), + text: item.data ? item.data.toString() : '',
91-93: Remove duplicate after hookThere are two identical
afterhooks that callapp.close(). This causes a linting error (noDuplicateTestHooks) and could lead to confusion or potential issues.- after(() => { - return app.close(); - });🧰 Tools
🪛 Biome (1.9.4)
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
95-160: DRY: Extract common test logic to a shared functionThe "sse should work" and "streamable should work" tests contain nearly identical logic with minor differences in transport types. Consider refactoring to extract the common test logic into a shared function.
+ async function testTransport( + transportType: 'sse' | 'streamable', + createTransport: (url: URL) => SSEClientTransport | StreamableHTTPClientTransport, + getBaseUrl: () => Promise<string> + ) { + const client = new Client({ + name: `${transportType}-demo-client`, + version: '1.0.0', + }); + const baseUrl = await getBaseUrl(); + const transport = createTransport(new URL(baseUrl)); + const notifications: { level: string, data: string }[] = []; + client.setNotificationHandler(LoggingMessageNotificationSchema, notification => { + notifications.push({ level: notification.params.level, data: notification.params.data as string }); + }); + await client.connect(transport); + const tools = await listTools(client); + const notificationResp = await startNotificationTool(client); + await new Promise(resolve => setTimeout(resolve, 5000)); + + if (transportType === 'streamable') { + await transport.terminateSession(); + await client.close(); + } else { + await transport.close(); + } + + assert.deepEqual(tools, [ + { + name: 'start-notification-stream', + description: 'Starts sending periodic notifications for testing resumability', + }, + ]); + assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); + assert.deepEqual(notifications, [ + { level: 'info', data: 'Periodic notification #1' }, + { level: 'info', data: 'Periodic notification #2' }, + { level: 'info', data: 'Periodic notification #3' }, + { level: 'info', data: 'Periodic notification #4' }, + { level: 'info', data: 'Periodic notification #5' }, + ]); + } it('sse should work', async () => { - const sseClient = new Client({ - name: 'sse-demo-client', - version: '1.0.0', - }); - const baseUrl = await app.httpRequest() - .get('/init').url; - const sseTransport = new SSEClientTransport(new URL(baseUrl)); - const sseNotifications: { level: string, data: string }[] = []; - sseClient.setNotificationHandler(LoggingMessageNotificationSchema, notification => { - sseNotifications.push({ level: notification.params.level, data: notification.params.data as string }); - }); - await sseClient.connect(sseTransport); - const tools = await listTools(sseClient); - const notificationResp = await startNotificationTool(sseClient); - await new Promise(resolve => setTimeout(resolve, 5000)); - await sseTransport.close(); - assert.deepEqual(tools, [ - { - name: 'start-notification-stream', - description: 'Starts sending periodic notifications for testing resumability', - }, - ]); - assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); - assert.deepEqual(sseNotifications, [ - { level: 'info', data: 'Periodic notification #1' }, - { level: 'info', data: 'Periodic notification #2' }, - { level: 'info', data: 'Periodic notification #3' }, - { level: 'info', data: 'Periodic notification #4' }, - { level: 'info', data: 'Periodic notification #5' }, - ]); + await testTransport( + 'sse', + url => new SSEClientTransport(url), + async () => await app.httpRequest().get('/init').url + ); }); it('streamable should work', async () => { - const streamableClient = new Client({ - name: 'streamable-demo-client', - version: '1.0.0', - }); - const baseUrl = await app.httpRequest() - .post('/stream').url; - const streamableTransport = new StreamableHTTPClientTransport(new URL(baseUrl)); - const streamableNotifications: { level: string, data: string }[] = []; - streamableClient.setNotificationHandler(LoggingMessageNotificationSchema, notification => { - streamableNotifications.push({ level: notification.params.level, data: notification.params.data as string }); - }); - await streamableClient.connect(streamableTransport); - const tools = await listTools(streamableClient); - const notificationResp = await startNotificationTool(streamableClient); - await new Promise(resolve => setTimeout(resolve, 5000)); - await streamableTransport.terminateSession(); - await streamableClient.close(); - assert.deepEqual(tools, [ - { - name: 'start-notification-stream', - description: 'Starts sending periodic notifications for testing resumability', - }, - ]); - assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); - assert.deepEqual(streamableNotifications, [ - { level: 'info', data: 'Periodic notification #1' }, - { level: 'info', data: 'Periodic notification #2' }, - { level: 'info', data: 'Periodic notification #3' }, - { level: 'info', data: 'Periodic notification #4' }, - { level: 'info', data: 'Periodic notification #5' }, - ]); + await testTransport( + 'streamable', + url => new StreamableHTTPClientTransport(url), + async () => await app.httpRequest().post('/stream').url + ); });plugin/controller/test/mcp/mcp.test.ts (3)
48-48: Add proper null checking instead of using non-null assertionUsing the non-null assertion operator (
!) can lead to runtime errors ifitem.datais actually null. Consider using proper null checking or providing a default value.- text: item.data!.toString(), + text: item.data ? item.data.toString() : '',
91-93: Remove duplicate after hookThere are two identical
afterhooks that callapp.close()(lines 61-63 and 91-93). This causes a linting error and could lead to confusion or potential issues.- after(() => { - return app.close(); - });🧰 Tools
🪛 Biome (1.9.4)
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
95-277: DRY: Extract common test logic between SSE and streamable testsBoth "sse should work" and "streamable should work" tests contain nearly identical logic with minor differences in transport types. Consider refactoring to create a common test function that takes the transport type and specific functions as parameters.
+ async function testMCPTransport( + transportType: 'sse' | 'streamable', + createTransport: (url: URL) => SSEClientTransport | StreamableHTTPClientTransport, + getEndpointUrl: () => Promise<string> + ) { + const client = new Client({ + name: `${transportType}-demo-client`, + version: '1.0.0', + }); + const baseUrl = await getEndpointUrl(); + const transport = createTransport(new URL(baseUrl)); + const notifications: { level: string, data: string }[] = []; + client.setNotificationHandler(LoggingMessageNotificationSchema, notification => { + notifications.push({ level: notification.params.level, data: notification.params.data as string }); + }); + await client.connect(transport); + + // Test tools + const tools = await listTools(client); + assert.deepEqual(tools, [ + { + name: 'start-notification-stream', + description: 'Starts sending periodic notifications for testing resumability', + }, + { + description: undefined, + name: 'bar', + }, + ]); + + const toolRes = await client.callTool({ + name: 'bar', + arguments: { + name: 'aaa', + }, + }); + assert.deepEqual(toolRes, { + content: [{ type: 'text', text: 'npm package: aaa not found' }], + }); + + // Test notifications + const notificationResp = await startNotificationTool(client); + await new Promise(resolve => setTimeout(resolve, 5000)); + assert.deepEqual(notificationResp, [{ text: 'Started sending periodic notifications every 1000ms' }]); + assert.deepEqual(notifications, [ + { level: 'info', data: 'Periodic notification #1' }, + { level: 'info', data: 'Periodic notification #2' }, + { level: 'info', data: 'Periodic notification #3' }, + { level: 'info', data: 'Periodic notification #4' }, + { level: 'info', data: 'Periodic notification #5' }, + ]); + + // Test resources + const resources = await client.listResources(); + assert.deepEqual(resources, { + resources: [ + { uri: 'mcp://npm/egg?version=4.10.0', name: 'egg' }, + { uri: 'mcp://npm/mcp?version=0.10.0', name: 'mcp' }, + ], + }); + + const resourceRes = await client.readResource({ + uri: 'mcp://npm/egg?version=4.10.0', + }); + assert.deepEqual(resourceRes, { + contents: [{ uri: 'mcp://npm/egg?version=4.10.0', text: 'MOCK TEXT' }], + }); + + // Test prompts + const prompts = await client.listPrompts(); + assert.deepEqual(prompts, { + prompts: [ + { name: 'foo', arguments: [{ name: 'name', required: true }] }, + ], + }); + + const promptRes = await client.getPrompt({ + name: 'foo', + arguments: { + name: 'bbb', + }, + }); + assert.deepEqual(promptRes, { + messages: [ + { + role: 'user', + content: { + type: 'text', + text: 'Generate a concise but descriptive commit message for these changes:\n\nbbb', + }, + }, + ], + }); + + // Close transport appropriately based on type + if (transportType === 'sse') { + await (transport as SSEClientTransport).close(); + } else { + await (transport as StreamableHTTPClientTransport).terminateSession(); + await client.close(); + } + } it('sse should work', async () => { - // [... existing implementation ...] + await testMCPTransport( + 'sse', + url => new SSEClientTransport(url), + async () => await app.httpRequest().get('/mcp/init').url + ); }); it('streamable should work', async () => { - // [... existing implementation ...] + await testMCPTransport( + 'streamable', + url => new StreamableHTTPClientTransport(url), + async () => await app.httpRequest().post('/mcp/stream').url + ); });plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (3)
192-193: 🛠️ Refactor suggestionAdd proper type checking for
detailbefore using non-null assertion.Using non-null assertion (
!) ondetailis risky ifdetailis null or undefined. Add proper null checking before usage.if (detail?.pid !== process.pid) { await this.app.mcpProxy.proxyMessage(this.ctx, { - detail: detail!, + detail, sessionId: this.ctx.request.query.sessionId, type: MCPProtocols.SSE, });
275-279: 🛠️ Refactor suggestionAdd proper null checking for
sidbefore using non-null assertion.Using non-null assertion (
!) onsidis risky ifsidis null or undefined. Add proper type checking instead.const sid = transport.sessionId; if (sid && transports[sid]) { delete transports[sid]; + await this.app.mcpProxy.unregisterClient(sid); + } else if (sid) { + await this.app.mcpProxy.unregisterClient(sid); } - await this.app.mcpProxy.unregisterClient(sid!);
296-302:⚠️ Potential issueAdd proper null checking for
detailbefore using non-null assertion.Using non-null assertion (
!) ondetailin line 299 is risky ifdetailis null or undefined. Add proper null checking before usage.const detail = await this.app.mcpProxy.getClient(sessionId); - if (detail?.pid !== process.pid) { + if (!detail) { + this.ctx.status = 404; + this.ctx.body = { + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session not found', + }, + id: null, + }; + return; + } else if (detail.pid !== process.pid) { await this.app.mcpProxy.proxyMessage(this.ctx, { - detail: detail!, + detail, sessionId, type: MCPProtocols.STREAM, });plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (9)
74-77: Document intended usage of the addHook method.The static
addHookmethod allows adding hooks to customize the MCP controller behavior, but there's no documentation on when and how to use it.+ /** + * Add a hook to customize the behavior of MCP controllers. + * Hooks can modify request/response handling, load schemas, or intercept proxy requests. + * + * @param hook MCPControllerHook implementation with optional lifecycle methods + */ static addHook(hook: MCPControllerHook) { MCPControllerRegister.hooks.push(hook); }
341-342: Use type-safe approach for registering handlers.The code uses unsafe type casting with
as unknown as [any, any, any, any]when calling MCP server methods, which bypasses type checking. Consider creating properly typed wrappers for the MCP server methods.A safer approach would be to use spread operators with explicit typing:
- this.mcpServer.prompt(...(args as unknown as [any, any, any, any])); + // Define the proper argument types + type PromptArgs = Parameters<typeof this.mcpServer.prompt>; + // Apply spread operator with explicit typing + this.mcpServer.prompt(...(args as PromptArgs));Apply similar changes to the other server method calls.
Also applies to: 385-386, 408-409
82-87: 🛠️ Refactor suggestionAdd error handling for hook execution.
Hook executions aren't wrapped in try/catch blocks, which could lead to unhandled promise rejections if a hook throws an exception.
if (MCPControllerRegister.hooks.length > 0) { for (const hook of MCPControllerRegister.hooks) { - await hook.preHandle?.(self.app.currentContext); + try { + await hook.preHandle?.(this.app.currentContext); + } catch (error) { + this.app.logger.error('Hook execution error:', error); + ctx.status = 500; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32603, + message: `Internal server error: ${error.message}`, + }, + id: null, + }; + return; + } } }
283-291:⚠️ Potential issueAdd error response for unknown session in message handler.
If a session ID is provided but not found in
self.transports, and no hook handles it, the function returns without providing an error response to the client.} if (MCPControllerRegister.hooks.length > 0) { for (const hook of MCPControllerRegister.hooks) { const checked = await hook.checkAndRunProxy?.(self.app.currentContext, MCPProtocols.SSE, sessionId); if (checked) { return; } } } + // If we reach here, the session was not found and no hook handled it + ctx.status = 404; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session not found', + }, + id: null, + };
381-383:⚠️ Potential issueFix the same argument handling bug in mcpToolRegister.
The same bug exists in the
mcpToolRegistermethod -newArgsis created but then not used in the finalReflect.applycall.- newArgs = [ ...newArgs, ...args ]; - return Reflect.apply(realMethod, realObj, newArgs); + // Fill any gaps in newArgs with undefined + const maxIndex = Math.max( + toolMeta.detail?.index ?? -1, + toolMeta.extra ?? -1 + ); + for (let i = 0; i <= maxIndex; i++) { + if (newArgs[i] === undefined) { + newArgs[i] = undefined; + } + } + return Reflect.apply(realMethod, realObj, newArgs);
432-439: 🛠️ Refactor suggestionFix initialization order to prevent potential race conditions.
mcpStreamServerInit,mcpServerInit, andmcpServerRegisterrely onthis.mcpServerbeing non-null, yet they are called immediately after creation and before awaiting any async work. If later code callsregister()concurrently, there's a race.if (!this.mcpServer) { this.mcpServer = new McpServer({ name: this.controllerMeta.name ?? `chair-mcp-${this.app.name}-server`, version: this.controllerMeta.version ?? '1.0.0', }, { capabilities: { logging: {} } }); - this.mcpStreamServerInit(); - this.mcpServerInit(); - this.mcpServerRegister(); + await Promise.all([ + this.mcpStreamServerInit(), + this.mcpServerInit(), + this.mcpServerRegister() + ]); }
179-181:⚠️ Potential issueAdd error response for unknown session.
If a session ID is provided but not found, and no hook handles it, the function returns without providing an error response to the client.
} } + // If we reach here, the session ID was provided but not found and no hook handled it + ctx.status = 404; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Session not found', + }, + id: null, + }; } return;
255-257:⚠️ Potential issueAdd validation for session ID.
The session ID is used without validation. If it's undefined, this can lead to errors when using it as an object key.
- const sessionId = ctx.query.sessionId; + const sessionId = ctx.query.sessionId as string | undefined; + if (!sessionId) { + ctx.status = 400; + ctx.body = { + jsonrpc: '2.0', + error: { + code: -32000, + message: 'Bad Request: sessionId is required', + }, + id: null, + }; + return; + }
28-41: 🛠️ Refactor suggestionAdd JSDoc comments to document the MCPControllerHook interface.
The
MCPControllerHookinterface defines several important lifecycle methods but lacks documentation explaining when each method is called and what it should do.export interface MCPControllerHook { + /** + * Called before initializing an SSE transport. + * Can be used to customize the transport or perform authentication. + * @param ctx Current request context + * @param transport The SSE transport being initialized + * @param register Reference to the MCPControllerRegister instance + */ // SSE preSSEInitHandle?: (ctx: Context, transport: SSEServerTransport, register: MCPControllerRegister) => Promise<void> + /** + * Called before handling an SSE message request. + * Can be used for authentication or request validation. + * @param ctx Current request context + */ preHandleInitHandle?: (ctx: Context) => Promise<void> + /** + * Called before handling a stream request. + * Can be used for authentication or request validation. + * @param ctx Current request context + */ // STREAM preHandle?: (ctx: Context) => Promise<void> + /** + * Called after a stream session is initialized. + * Can be used for session setup or custom logic. + * @param ctx Current request context + * @param transport The stream transport that was initialized + * @param register Reference to the MCPControllerRegister instance + */ onStreamSessionInitialized?: (ctx: Context, transport: StreamableHTTPServerTransport, register: MCPControllerRegister) => Promise<void> + /** + * Called before proxying a request. + * Can be used to modify the proxy request or response. + * @param ctx Current request context + * @param proxyReq The proxy request + * @param proxyResp The proxy response + */ // COMMON preProxy?: (ctx: Context, proxyReq: http.IncomingMessage, proxyResp: http.ServerResponse) => Promise<void> + /** + * Called to load schemas for prompts or tools. + * Can be used to dynamically load schemas from external sources. + * @param controllerMeta Controller metadata + * @param meta Prompt or tool metadata + * @returns Schema for the prompt or tool + */ schemaLoader?: (controllerMeta: MCPControllerMeta, meta: MCPPromptMeta | MCPToolMeta) => Promise<Parameters<McpServer['tool']>['2'] | Parameters<McpServer['prompt']>['2']> + /** + * Called to check if a proxy can handle a request. + * Can be used to delegate request handling to a proxy. + * @param ctx Current request context + * @param type Protocol type (SSE or STREAM) + * @param sessionId Session ID + * @returns True if the proxy handled the request, false otherwise + */ checkAndRunProxy?: (ctx: Context, type: MCPProtocols, sessionId: string) => Promise<boolean>; }
🧹 Nitpick comments (9)
core/controller-decorator/src/decorator/mcp/MCPController.ts (1)
18-24: Remove redundant comments showing import pathsThese comments appear to be debug information about import paths and don't add value to the code. Consider removing them for a cleaner implementation.
- // './tegg/core/common-util/src/StackUtil.ts', - // './tegg/core/core-decorator/src/decorator/Prototype.ts', - // './tegg/core/controller-decorator/src/decorator/tr/TRController.ts', - // './tegg/core/core-decorator/node_modules/[email protected]@reflect-metadata/Reflect.js', - // './tegg/core/core-decorator/node_modules/[email protected]@reflect-metadata/Reflect.js', - // './tegg/core/controller-decorator/test/fixtures/TRFooController.ts', PrototypeUtil.setFilePath(clazz, StackUtil.getCalleeFromStack(false, 5));plugin/controller/test/mcp/mcp.test.ts (1)
58-58: Add Node.js version check explanationThe test is conditionally run only on Node.js versions greater than 17. Consider adding a comment explaining why this version restriction exists to help future maintainers understand the constraints.
- if (parseInt(process.version.slice(1, 3)) > 17) { + // MCP functionality requires Node.js version > 17 due to dependencies on newer features + if (parseInt(process.version.slice(1, 3)) > 17) {plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (5)
171-172: Remove unnecessarythisaliasing.The
const self = thisdeclaration is unnecessary in this context since arrow functions inheritthisfrom their enclosing scope.- const self = this; - const transport = new SSEServerTransport('/message', this.ctx.res); + const transport = new SSEServerTransport('/message', this.ctx.res);🧰 Tools
🪛 Biome (1.9.4)
[error] 171-171: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
174-176: Improve type safety in proxy handler function.The transport variable is accessible directly from the outer scope, so there's no need to use
selfhere.this.app.mcpProxy.setProxyHandler(MCPProtocols.SSE, async (req, res) => { - return await self.ssePostHandler(req, res); + return await this.ssePostHandler(req, res); });
208-212: Improve error message for missing session ID.The error message "session id not have and run in proxy" is not grammatically correct and could be clearer.
if (!sessionId) { res.status(500).json({ jsonrpc: '2.0', error: { code: -32603, - message: 'session id not have and run in proxy', + message: 'Session ID is required but not provided', }, id: null, });
260-262: Remove unnecessarythisaliasing.The
const self = thisdeclaration is unnecessary in this context since arrow functions inheritthisfrom their enclosing scope.- const self = this; const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID(), eventStore, onsessioninitialized: async sessionId => {🧰 Tools
🪛 Biome (1.9.4)
[error] 261-261: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
267-269: Improve type safety in proxy handler function.Similar to the SSE handler, there's no need to use
selfhere.this.app.mcpProxy.setProxyHandler(MCPProtocols.STREAM, async (req, res) => { - return await self.streamPostHandler(req, res); + return await this.streamPostHandler(req, res); });core/controller-decorator/src/util/MCPInfoUtil.ts (1)
40-156: Consider using namespaced functions instead of a static-only class.This class contains only static members, which is generally an anti-pattern. Consider using simple exported functions or a TypeScript namespace instead of a class with only static methods.
For example, you could refactor this to:
export namespace MCPInfoUtil { export function setMCPName(name: string, clazz: EggProtoImplClass) { MetadataUtil.defineMetaData(CONTROLLER_MCP_NAME, name, clazz); } export function getMCPName(clazz: EggProtoImplClass): string | undefined { return MetadataUtil.getMetaData(CONTROLLER_MCP_NAME, clazz); } // ... other functions }Or use individual exported functions:
export function setMCPName(name: string, clazz: EggProtoImplClass) { MetadataUtil.defineMetaData(CONTROLLER_MCP_NAME, name, clazz); } export function getMCPName(clazz: EggProtoImplClass): string | undefined { return MetadataUtil.getMetaData(CONTROLLER_MCP_NAME, clazz); } // ... other functionsThis would align better with modern JavaScript/TypeScript practices and avoid the class-that-is-not-a-class pattern.
🧰 Tools
🪛 Biome (1.9.4)
[error] 40-156: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
81-81: Remove unnecessary this aliasing.Arrow functions inherit
thisfrom their enclosing scope, making the alias unnecessary.- const self = this;The code would need to be updated throughout to replace
selfwiththis.🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
core/controller-decorator/index.ts(2 hunks)core/controller-decorator/src/decorator/mcp/Extra.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPController.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPPrompt.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPResource.ts(1 hunks)core/controller-decorator/src/decorator/mcp/MCPTool.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerResourceMetaBuilder.ts(1 hunks)core/controller-decorator/src/impl/mcp/MCPControllerToolMetaBuilder.ts(1 hunks)core/controller-decorator/src/model/MCPPromptMeta.ts(1 hunks)core/controller-decorator/src/model/MCPResourceMeta.ts(1 hunks)core/controller-decorator/src/model/MCPToolMeta.ts(1 hunks)core/controller-decorator/src/util/MCPInfoUtil.ts(1 hunks)core/controller-decorator/test/MCPMeta.test.ts(1 hunks)core/controller-decorator/test/fixtures/MCPController.ts(1 hunks)core/types/controller-decorator/MCPController.ts(1 hunks)core/types/controller-decorator/MCPPromptParams.ts(1 hunks)core/types/controller-decorator/MCPToolParams.ts(1 hunks)core/types/controller-decorator/MetadataKey.ts(1 hunks)core/types/package.json(1 hunks)plugin/controller/app.ts(2 hunks)plugin/controller/app/middleware/mcp_body_middleware.ts(1 hunks)plugin/controller/config/config.default.ts(1 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js(1 hunks)plugin/controller/test/mcp/mcp.test.ts(1 hunks)plugin/mcp-proxy/agent.ts(1 hunks)plugin/mcp-proxy/app.ts(1 hunks)plugin/mcp-proxy/config/config.default.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)plugin/mcp-proxy/package.json(1 hunks)plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts(1 hunks)plugin/mcp-proxy/test/proxy.test.ts(1 hunks)plugin/mcp-proxy/typings/index.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- core/controller-decorator/src/decorator/mcp/Extra.ts
- core/controller-decorator/src/model/MCPResourceMeta.ts
- core/types/controller-decorator/MCPPromptParams.ts
- plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- plugin/mcp-proxy/config/config.default.ts
- core/types/package.json
- plugin/controller/app/middleware/mcp_body_middleware.ts
- core/controller-decorator/src/decorator/mcp/MCPResource.ts
- plugin/mcp-proxy/agent.ts
- plugin/mcp-proxy/app.ts
- core/types/controller-decorator/MCPController.ts
- core/controller-decorator/src/impl/mcp/MCPControllerToolMetaBuilder.ts
- plugin/controller/app.ts
- plugin/controller/config/config.default.ts
- core/controller-decorator/src/model/MCPToolMeta.ts
- core/controller-decorator/src/impl/mcp/MCPControllerPromptMetaBuilder.ts
- plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts
- plugin/mcp-proxy/typings/index.d.ts
- core/controller-decorator/src/model/MCPPromptMeta.ts
- core/controller-decorator/src/impl/mcp/MCPControllerResourceMetaBuilder.ts
- plugin/mcp-proxy/package.json
- core/controller-decorator/test/MCPMeta.test.ts
- core/types/controller-decorator/MCPToolParams.ts
- core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts
- core/controller-decorator/src/decorator/mcp/MCPPrompt.ts
- core/controller-decorator/src/decorator/mcp/MCPTool.ts
- core/controller-decorator/index.ts
- core/controller-decorator/test/fixtures/MCPController.ts
- core/types/controller-decorator/MetadataKey.ts
- plugin/mcp-proxy/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
core/controller-decorator/src/decorator/mcp/MCPController.ts
[error] 8-8: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
core/controller-decorator/src/util/MCPInfoUtil.ts
[error] 40-156: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 81-81: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 196-196: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 250-250: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 319-319: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 365-365: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 402-402: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
plugin/controller/test/mcp/mcp.test.ts
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts
[error] 171-171: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 261-261: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
plugin/mcp-proxy/test/proxy.test.ts
[error] 91-93: Disallow duplicate setup and teardown hooks.
Disallow after duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
🔇 Additional comments (2)
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js (1)
1-27: The plugin configuration looks good!All plugin configurations are properly defined with unique export names and have consistent structure. The test application is configured to use the necessary plugins for testing MCP functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
core/controller-decorator/src/util/MCPInfoUtil.ts (1)
117-124: The method now correctly uses CONTROLLER_MCP_PROMPT_PARAMS_MAP.The implementation correctly uses the separate
CONTROLLER_MCP_PROMPT_PARAMS_MAPconstant to store prompt parameters, which prevents collision with resource parameters. This is a good fix from previous iterations.
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores