fix: sse new ctx 8000 #323
fix: sse new ctx #323
Conversation
WalkthroughThis update introduces a controller hook system for the MCP controller by modifying the SSE context handling to invoke registered Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPControllerRegister
participant HookPlugin
participant Controller
Client->>MCPControllerRegister: Send SSE request
MCPControllerRegister->>MCPControllerRegister: Create new context with all headers
MCPControllerRegister->>HookPlugin: Call preHandle(ctx) for each registered hook
HookPlugin->>MCPControllerRegister: (Inject user info into ctx)
MCPControllerRegister->>Controller: Pass context to middleware and handler
Controller->>Client: Respond (e.g., echoUser greets user)
Possibly related PRs
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/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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/package.json (1)
1-6: LGTM! Consider adding version field for completeness.The package.json structure correctly follows Egg.js plugin conventions with the proper
eggPluginconfiguration.Consider adding a version field for better package management:
{ "name": "@eggjs/hook-plugin", + "version": "1.0.0", "eggPlugin": { "name": "hookPlugin" } }plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js (1)
2-3: Consider using consistent import style for the path module.The functionality is correct, but for consistency with other plugin files in the codebase, consider using the Node.js built-in module specifier.
Based on the pattern used in
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/plugin.js(line 4), consider this change:-// eslint-disable-next-line @typescript-eslint/no-var-requires -const path = require('path'); +const path = require('node:path');This follows the modern Node.js convention for built-in modules and eliminates the need for the eslint disable comment.
plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/app.ts (1)
1-15: Well-structured boot hook implementation.The boot hook correctly follows Egg.js patterns and uses the appropriate lifecycle method for hook registration.
Consider adding error handling for hook registration to make the system more robust:
configWillLoad() { - MCPControllerRegister.addHook(GetAlipayTeggHook(this.#app)); + try { + MCPControllerRegister.addHook(GetAlipayTeggHook(this.#app)); + } catch (error) { + this.#app.logger.error('Failed to register MCP controller hook:', error); + throw error; + } }Note: The function name
GetAlipayTeggHookappears specific to Alipay. If this is intended for general use, consider a more generic name likeGetUserInjectionHook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts(2 hunks)plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js(2 hunks)plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/app.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/lib/MCPControllerHook.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/package.json(1 hunks)plugin/controller/test/mcp/mcp.test.ts(9 hunks)plugin/controller/test/mcp/mcpCluster.test.ts(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js (1)
plugin/mcp-proxy/test/fixtures/apps/mcp-proxy/config/plugin.js (1)
path(4-4)
plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/lib/MCPControllerHook.ts (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
MCPControllerHook(31-44)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (2)
core/controller-decorator/src/decorator/mcp/MCPTool.ts (1)
MCPTool(10-33)core/types/controller-decorator/MCPToolParams.ts (1)
MCPToolResponse(8-8)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (8)
plugin/controller/test/fixtures/apps/mcp-app/config/plugin.js (1)
31-34: Plugin configuration looks good.The hookPlugin configuration correctly follows Egg.js plugin patterns with proper path resolution and enablement.
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
400-403:❓ Verification inconclusive
Verify security implications of propagating all headers.
The change from setting specific headers to spreading all headers (
...ctx.req.headers) could potentially expose sensitive information. Consider filtering or whitelisting specific headers instead of propagating everything.
🏁 Script executed:
#!/bin/bash # Check if there are any header filtering mechanisms or security considerations documented rg -A 5 -B 5 "headers.*security|sensitive.*header|filter.*header" --type tsLength of output: 77
Review header propagation security
I didn’t find any existing header‐filtering or whitelist mechanism in the codebase. Spreading all incoming headers (
...ctx.req.headers) risks leaking sensitive data.• File/plugin:
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(lines 400–403)
• No “allowed headers” list or sanitization logic detected
• Suggest explicitly selecting or filtering headers (e.g. only propagateaccept,content-type, and any required auth headers) rather than spreading everythingPlease verify that only safe headers are forwarded or implement a header whitelist before propagation.
plugin/controller/test/mcp/mcpCluster.test.ts (3)
103-125: LGTM! Auth provider implementation is consistent and well-structured.The auth provider configuration is properly implemented across all three transport types with:
- Consistent structure and mock implementations
- Proper base64 encoding of test token
- Appropriate no-op functions for unused auth methods
The test setup enables proper verification of the hook functionality.
Also applies to: 227-250, 359-381
142-145: LGTM! Test coverage for new echoUser tool is comprehensive.The addition of
echoUserto the expected tools list is consistent across all transport types and properly tests the new hook functionality.Also applies to: 268-271, 398-401
157-163: LGTM! echoUser tool invocation tests are well-structured.The test calls to
echoUsertool properly verify:
- Tool accepts empty arguments
- Returns expected greeting message with injected user
- Validates integration between auth provider, hook, and controller
Also applies to: 283-289, 413-419
plugin/controller/test/mcp/mcp.test.ts (3)
93-115: LGTM! Auth provider implementation matches cluster tests.The auth provider configuration is identical to the cluster test file, ensuring consistent test coverage across both single-process and multi-process scenarios.
Also applies to: 218-241, 351-373
132-135: LGTM! Tool list assertions are consistent with cluster tests.The
echoUsertool addition maintains consistency between test files and properly validates the new hook functionality.Also applies to: 259-262, 390-393
148-154: LGTM! echoUser tool tests validate end-to-end functionality.The test calls confirm that the authentication hook properly injects user context and that the controller method returns the expected greeting message.
Also applies to: 275-281, 406-412
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts
Show resolved
Hide resolved
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts
Show resolved
Hide resolved
plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/lib/MCPControllerHook.ts
Show resolved
Hide resolved
plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/lib/MCPControllerHook.ts
Show resolved
Hide resolved
plugin/controller/test/fixtures/apps/mcp-app/hook-plugin/lib/MCPControllerHook.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugin/mcp-proxy/index.ts (3)
69-76: Approve the response format change but verify content-type header.The change from Express-style to raw HTTP responses provides better control and is consistent with the low-level nature of the proxy.
Consider adding a content-type header for consistency:
- res.writeHead(400).end(JSON.stringify({ + res.writeHead(400, { 'content-type': 'application/json' }).end(JSON.stringify({This matches the pattern used elsewhere in the file (lines 130, 145, 164) and ensures proper content-type handling.
97-104: Approve the response format change but add content-type header for consistency.The error response structure and JSON-RPC 2.0 format are correct.
Add content-type header to match the pattern used elsewhere in the file:
- res.writeHead(404).end(JSON.stringify({ + res.writeHead(404, { 'content-type': 'application/json' }).end(JSON.stringify({This ensures consistent content-type handling across all error responses.
69-104: Consider extracting error response helper function.The JSON-RPC error response pattern is repeated multiple times throughout the file with slight variations.
Consider creating a helper function to reduce duplication:
function sendJsonRpcError(res: http.ServerResponse, statusCode: number, err 8000 orCode: number, message: string, id: any = null) { res.writeHead(statusCode, { 'content-type': 'application/json' }).end(JSON.stringify({ jsonrpc: '2.0', error: { code: errorCode, message, }, id, })); } // Usage: sendJsonRpcError(res, 400, -32000, 'Bad Request: Session exists but uses a different transport protocol'); sendJsonRpcError(res, 404, -32602, 'Bad Request: No transport found for sessionId');This would improve maintainability and ensure consistent error response formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/mcp-proxy/index.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (18)
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit