feat: add mcp stateless#309
Conversation
WalkthroughThis update introduces stateless stream transport support for MCP controllers, centralizes MCP configuration in a new Changes
Sequence Diagram(s)sequenceDiagram
participant AppBootHook
participant MCPControllerRegister
participant MCPConfig
participant ExpressApp
participant Client
AppBootHook->>MCPControllerRegister: doRegister()
AppBootHook->>MCPControllerRegister: connectStatelessStreamTransport() (if mcpProxy enabled)
MCPControllerRegister->>MCPConfig: Load configuration (including statelessStreamPath)
MCPControllerRegister->>ExpressApp: Register /mcp/stateless/stream POST route
Client->>ExpressApp: POST /mcp/stateless/stream (stateless stream request)
ExpressApp->>MCPControllerRegister: Handle request via StreamableHTTPServerTransport
MCPControllerRegister-->>Client: Responds with streamable/stateless data
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
plugin/mcp-proxy/app.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. plugin/controller/lib/impl/mcp/MCPControllerRegister.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. plugin/mcp-proxy/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.
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
plugin/controller/test/mcp/mcp.test.ts (1)
279-370: Test case is comprehensive but contains duplicated codeThe test case thoroughly verifies all aspects of the stateless stream transport functionality, which is excellent. It properly tests tools, notifications, resources, and prompts interactions.
Consider refactoring the "streamable should work" and "stateless streamable should work" test cases to reduce code duplication. You could extract a common testing function that accepts the endpoint URL as a parameter:
- 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; - // ... rest of test code - }); - - it('stateless streamable should work', async () => { - const streamableClient = new Client({ - name: 'streamable-demo-client', - version: '1.0.0', - }); - const baseUrl = await app.httpRequest() - .post('/mcp/stateless/stream').url; - // ... rest of test code - }); + async function testStreamable(endpoint, testName) { + it(`${testName} should work`, async () => { + const streamableClient = new Client({ + name: 'streamable-demo-client', + version: '1.0.0', + }); + const baseUrl = await app.httpRequest() + .post(endpoint).url; + // ... rest of test code + }); + } + + testStreamable('/mcp/stream', 'streamable'); + testStreamable('/mcp/stateless/stream', 'stateless streamable');This would make the tests more maintainable and reduce the risk of inconsistencies when implementing future changes.
plugin/controller/README.md (2)
239-246: Incorrect Zod import in the sample code
zoddoes not expose a default export; attemptingimport z from 'zod'will raiseTS1192: Module 'zod' has no default export.
Use the named export instead:-import z from 'zod'; +import { z } from 'zod';Readers copy-pasting the sample will otherwise get a compile-time error.
214-222: Minor language/formatting issues reduce readabilityThe Chinese/English mix (“使用
@ToolArgsSchema@PromptArgsSchema@Extra来 schema 和 extra”) is a bit hard to parse.
Consider re-phrasing:使用
@ToolArgsSchema、@PromptArgsSchema和@Extra来声明参数 schema 与额外元数据。No functional impact, but clearer docs help onboarding.
plugin/controller/lib/impl/mcp/MCPConfig.ts (1)
34-60: Explicit return types improve IDE feedbackAll getters currently rely on inference. Declaring return types is a tiny effort that improves DX and prevents accidental changes:
- get sseInitPath() { + get sseInitPath(): string {Repeat for all getters.
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (3)
125-129: Usectxinstead ofself.app.currentContextInside the route handler you already have
ctx; storingself.app.currentContextis unnecessary and triggered Biome’snoUselessThisAlias.
Just passctxto hooks:- await hook.preHandle?.(self.app.currentContext); + await hook.preHandle?.(ctx);Applies to multiple handlers (
preHandle,onStreamSessionInitialized, etc.).
175-218: Optional chaining simplifies deep-hook traversalBiome flagged the manual
length > 0check.
You can condense:-if (MCPControllerRegister.hooks.length > 0) { - for (const hook of MCPControllerRegister.hooks) { - await hook.preHandle?.(self.app.currentContext); - } -} +for (const hook of MCPControllerRegister.hooks ?? []) { + await hook.preHandle?.(ctx); +}Reduces nesting and noise.
🧰 Tools
🪛 Biome (1.9.4)
[error] 179-180: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
280-282: Use configuredstreamPathonce, avoid alias
selfaliasing is noisy and error-prone (Biome hint). Arrow functions inheritthis; you can dropselfand accessthis.mcpConfig.streamPathdirectly insideinitHandler, or captureconst { mcpConfig } = this;.Not blocking, but consider cleaning up for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
plugin/controller/README.md(1 hunks)plugin/controller/app.ts(1 hunks)plugin/controller/app/middleware/mcp_body_middleware.ts(1 hunks)plugin/controller/config/config.default.ts(1 hunks)plugin/controller/lib/impl/mcp/MCPConfig.ts(1 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(11 hunks)plugin/controller/test/mcp/mcp.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
plugin/controller/app.ts (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
MCPControllerRegister(46-547)
plugin/controller/lib/impl/mcp/MCPConfig.ts (1)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/app/controller/app.ts (1)
InMemoryEventStore(59-127)
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 120-120: 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] 294-294: 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: Runner-macos (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (3)
plugin/controller/config/config.default.ts (1)
10-10: LGTM: Config addition follows existing naming conventionsThe new
statelessStreamPathconfiguration property is correctly added to themcpconfig object, maintaining consistent path structure with other MCP endpoints.plugin/controller/app/middleware/mcp_body_middleware.ts (1)
6-6: Properly updated middleware to handle new endpointAdding the stateless stream path to the body parser exclusion list ensures consistent behavior with other MCP streaming endpoints.
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
150-172: Duplicate route-name strings may shadow each other
Reflect.applyregisters three routes (POST/GET/DEL) all named'chairMcpStatelessStreamInit'.
Egg exploits the name as a unique identifier; re-using it with different HTTP verbs risks the later calls overwriting the first, depending on router internals.Prefer unique names, e.g.:
- 'chairMcpStatelessStreamInit', + 'chairMcpStatelessStreamInitPost',(and change accordingly for GET/DEL).
akitaSummer
force-pushed
the
feat/mcp-stateless
branch
from
34839b9 to
fe0fd58
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
120-122:⚠️ Potential issuePass
eventStore&sessionIdGeneratorinto stateless transportUnlike the stream mode, the stateless transport is created without explicit options for eventStore and sessionIdGenerator.
This loses the custom
eventStoreandsessionIdGeneratorconfigured inMCPConfig.const transport: StreamableHTTPServerTransport = new StreamableHTTPServerTransport({ - sessionIdGenerator: undefined, + sessionIdGenerator: () => this.mcpConfig.sessionIdGenerator(), + eventStore: this.mcpConfig.eventStore, });🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: 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 (4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (4)
85-115: Implementation of stateless transport connection and initialization.The static method correctly connects the MCP server to the stateless transport and performs an initialization request with appropriate protocol details.
However, there's an opportunity to use optional chaining for more concise code.
- if (MCPControllerRegister.instance && MCPControllerRegister.instance.statelessTransport) { + if (MCPControllerRegister.instance?.statelessTransport) {🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
120-120: Remove unnecessarythisaliasing.Arrow functions inherit
thisfrom their enclosing scope, making this aliasing unnecessary.- const self = this; - const transport: StreamableHTTPServerTransport = new StreamableHTTPServerTransport({ + const transport: StreamableHTTPServerTransport = new StreamableHTTPServerTransport({And then use
thisinstead ofselfin the following code.🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
294-294: Remove unnecessarythisaliasing.Similar to the issue at line 120, this
selfvariable is redundant as arrow functions inheritthis.- const transport = new SSEServerTransport(self.mcpConfig.sseMessagePath, ctx.res); + const transport = new SSEServerTransport(this.mcpConfig.sseMessagePath, ctx.res);🧰 Tools
🪛 Biome (1.9.4)
[error] 294-294: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
117-173: Consider refactoring long methods for better maintainability.The
mcpStatelessStreamServerInitmethod is quite long and performs multiple responsibilities. Consider breaking it down into smaller, focused methods to improve readability and maintainability.For example, you could extract the handler functions and router registration into separate methods:
private createStatelessStreamHandler(): (ctx: Context) => Promise<void> { // Extract the initHandler logic } private createMethodNotAllowedHandler(): (ctx: Context) => Promise<void> { // Extract the notHandler logic } private registerStatelessStreamRoutes( initHandler: (ctx: Context) => Promise<void>, notHandler: (ctx: Context) => Promise<void> ) { // Register the routes } mcpStatelessStreamServerInit() { const initHandler = this.createStatelessStreamHandler(); const notHandler = this.createMethodNotAllowedHandler(); this.registerStatelessStreamRoutes(initHandler, notHandler); }🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: 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 (2)
plugin/controller/app.ts(3 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/app.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 120-120: 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] 294-294: 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 (6)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (16)
🔇 Additional comments (9)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (9)
4-5: Appropriate imports added for stateless transport implementation.The new imports from 'node:http' and 'node:net' are necessary for constructing the synthetic HTTP request used to initialize the stateless stream transport.
28-28: Good practice: Centralizing configuration in MCPConfig.Centralizing configuration in a dedicated class improves code organization and maintainability.
57-58: Class property additions correctly setup for stateless transport.The new properties store the centralized configuration and stateless transport instance needed for the new feature.
78-78: Configuration initialization looks good.Properly initializes the MCPConfig instance with the app's MCP configuration.
94-94: Correct usage of configured stateless stream path.The code now correctly uses the path from MCPConfig, addressing a previous issue where the wrong path was hardcoded.
147-172: Good error handling for unsupported HTTP methods.The implementation properly handles unsupported HTTP methods with a 405 status code and clear error message, which is a good security practice.
213-214: Properly using centralized config for stream transport.The code now correctly uses the MCPConfig properties for eventStore and sessionIdGenerator in the stream transport, providing consistency and centralized configuration management.
Also applies to: 216-217
309-309: Using centralized config for SSE heartbeat timing.Correctly uses the MCPConfig for the SSE heartbeat interval, which improves configurability.
532-532: Initialization of stateless stream server in register method.The register method now correctly initializes the stateless stream server alongside the existing stream and SSE servers.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
121-123: PasseventStore&sessionIdGeneratorinto stateless transportUnlike the stream mode, the stateless transport is created without explicit options:
const transport: StreamableHTTPServerTransport = new StreamableHTTPServerTransport({ - sessionIdGenerator: undefined, + sessionIdGenerator: () => this.mcpConfig.sessionIdGenerator(), + eventStore: this.mcpConfig.eventStore, });Keeps behavior consistent across transport types.
🧰 Tools
🪛 Biome (1.9.4)
[error] 121-121: 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 (4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (4)
87-115: Ensure optional chaining for better robustnessThe conditional check could be simplified using optional chaining for better maintainability.
- if (MCPControllerRegister.instance && MCPControllerRegister.instance.statelessTransport) { + if (MCPControllerRegister.instance?.statelessTransport) {🧰 Tools
🪛 Biome (1.9.4)
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
121-121: Remove unnecessary this aliasing in arrow function contextArrow functions inherit
thisfrom their enclosing scope, making theselfvariable unnecessary.- const self = this;🧰 Tools
🪛 Biome (1.9.4)
[error] 121-121: 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-131: Move hook processing logic to a shared helper functionThe hook processing logic is duplicated in several places. Consider extracting it to a helper method to reduce duplication.
+ private async processHooks(ctx: Context, hookMethod: keyof MCPControllerHook) { + if (MCPControllerRegister.hooks.length > 0) { + for (const hook of MCPControllerRegister.hooks) { + await hook[hookMethod]?.(ctx); + } + } + } // Then use it like: // await this.processHooks(self.app.currentContext, 'preHandle');
295-295: Remove unnecessary this aliasingThe variable
selfis redundant as arrow functions inheritthisfrom their enclosing scope.- const transport = new SSEServerTransport(self.mcpConfig.sseMessagePath, ctx.res); + const transport = new SSEServerTransport(this.mcpConfig.sseMessagePath, ctx.res);🧰 Tools
🪛 Biome (1.9.4)
[error] 295-295: 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 (7)
plugin/controller/README.md(1 hunks)plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(14 hunks)plugin/controller/package.json(1 hunks)plugin/mcp-proxy/app.ts(2 hunks)plugin/mcp-proxy/app/extend/agent.ts(1 hunks)plugin/mcp-proxy/index.ts(1 hunks)plugin/mcp-proxy/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- plugin/controller/package.json
- plugin/mcp-proxy/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/README.md
🧰 Additional context used
🪛 GitHub Check: CodeQL
plugin/mcp-proxy/index.ts
[failure] 230-230: Unvalidated dynamic method call
Invocation of method with user-controlled name may dispatch to unexpected target and cause an exception.
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 87-87: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 121-121: 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)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (5)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
536-540: LGTM: Good initialization of both serversThe code properly initializes both the regular MCP server and the new stateless MCP server with consistent configurations.
plugin/mcp-proxy/app/extend/agent.ts (1)
9-14: LGTM: Setting isAgent flag for agent contextThe addition of
isAgent: trueis appropriate for the agent context and aligns with the changes in theMCPProxyApiClientclass to conditionally start the HTTP server.plugin/mcp-proxy/app.ts (1)
12-14: LGTM: Early hook registration in the application lifecycleEarly registration of
MCPProxyHookin theconfigWillLoadmethod ensures the hook is available when MCP controllers are initialized.plugin/mcp-proxy/index.ts (2)
55-203: LGTM: Well-organized extraction of MCPProxyHookThe extraction of
MCPProxyHookto a standalone exported constant improves code organization and modularity. This separation of concerns allows the hook to be registered separately from the client instance.
227-238: LGTM: Conditional server initialization based on agent modeThe conditional initialization of the HTTP server based on the
isAgentflag is appropriate and integrates well with the changes in the agent.ts file.🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 230-230: Unvalidated dynamic method call
Invocation of method with user-controlled name may dispatch to unexpected target and cause an exception.
3 participants
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit