fix: mcp teggCtxLifecycleMiddleware #312
Conversation
## Walkthrough
The changes standardize context creation and introduce the `teggCtxLifecycleMiddleware` middleware to wrap asynchronous transport request handling in both the controller and proxy layers. Direct instantiation of context objects is replaced with the app's context creation method, and middleware is now consistently applied during transport operations.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------|
| plugin/controller/lib/impl/mcp/MCPControllerRegister.ts | Added `teggCtxLifecycleMiddleware` to wrap calls to transport request handlers in several methods, ensuring middleware is applied to requests. |
| plugin/mcp-proxy/index.ts | Replaced manual context instantiation with `app.createContext`; applied `teggCtxLifecycleMiddleware` around transport handling in proxies. |
| core/types/controller-decorator/MCPToolParams.ts | Updated `ToolExtra` type alias to reference a different parameter index in `McpServer['tool']` type parameters. |
| plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts | Added injected `Logger` and logged a message after notification stream completion. |
| plugin/controller/test/mcp/mcp.test.ts | Added log file reading and assertion to verify the presence of a specific log entry after streamable transport test completion. |
| plugin/controller/test/fixtures/apps/mcp-app/package.json | Renamed package from `"controller-app"` to `"mcp-app"`. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant Proxy
participant App
participant Middleware
participant Transport
Client->>Proxy: Send request (SSE/STREAM)
Proxy->>App: app.createContext(req, res)
Proxy->>Middleware: teggCtxLifecycleMiddleware()
Middleware->>Transport: handleRequest(ctx)
Transport-->>Middleware: Response
Middleware-->>Proxy: Response
Proxy-->>Client: ResponsePossibly related PRs
Suggested reviewers
Poem
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
125-126: Single shared middleware instance – same caveat as in proxy
const mw = self.app.middleware.teggCtxLifecycleMiddleware();is created once per server start, not per HTTP request.
Make sure the middleware is stateless or instantiate it on-demand (see comment inplugin/mcp-proxy/index.ts).
🧹 Nitpick comments (3)
plugin/mcp-proxy/index.ts (2)
60-66: Shadowing the outerctxparameter can be confusingInside the SSE proxy handler you redeclare
const ctxwhich shadows thectxreceived bypreSSEInitHandle.
Although technically safe, it makes reasoning about which context is being referenced harder for future readers.Consider renaming the inner variable to something like
proxyCtx(or similar) to avoid confusion.
125-129:teggCtxLifecycleMiddlewareinstance is created once – verify it is stateless
const mw = self.app.middleware.teggCtxLifecycleMiddleware();is executed once when the proxy handler is registered, then reused for every incoming request.
If the middleware keeps per-request state in closure variables, re-using the same function could introduce cross-request leakage.
- Double-check that
teggCtxLifecycleMiddlewarereturns a pure(ctx, next)handler that stores all per-request data on the passedctxobject.- If it is not guaranteed to be stateless, instantiate it per request:
-const mw = self.app.middleware.teggCtxLifecycleMiddleware(); ... await self.app.ctxStorage.run(ctx, async () => { - await mw(ctx, async () => { ... }); + const mw = self.app.middleware.teggCtxLifecycleMiddleware(); + await mw(ctx, async () => { ... }); });plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
182-183: Duplicate code – consider extracting helperYou now have at least three places where you repeat:
const mw = app.middleware.teggCtxLifecycleMiddleware(); ... await ctx.app.ctxStorage.run(ctx, async () => { await mw(ctx, async () => { /* … */ }); });Extracting a small helper (e.g.,
runWithCtxLifecycle(ctx, fn)) would reduce duplication and risk of inconsistency.🧰 Tools
🪛 Biome (1.9.4)
[error] 183-183: 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/lib/impl/mcp/MCPControllerRegister.ts(7 hunks)plugin/mcp-proxy/index.ts(3 hunks)
⏰ 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 (18)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (16)
🔇 Additional comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
338-352:❌ Incorrect review comment
Potential race conditions in
sseCtxStorageRun
mwis allocated once persseCtxStorageRuncall and reused for every incoming SSE message on that transport.
Because multiple messages can be processed concurrently, middleware that stores state on function-level variables could leak between messages.Instantiate middleware inside the
onmessagewrapper instead:transport. (...args) => { await ctx.app.ctxStorage.run(ctx, async () => { - await mw(ctx, async () => { await messageFunc!(...args); }); + const mw = this.app.middleware.teggCtxLifecycleMiddleware(); + await mw(ctx, async () => { await messageFunc!(...args); }); }); };
I want to inspect whether
teggCtxLifecycleMiddlewarereturns stateful closures, so let’s locate its implementation. Then we can confirm if re-instantiating it per message is necessary.
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Searching for teggCtxLifecycleMiddleware occurrences..." rg -n "teggCtxLifecycleMiddleware" . echo echo "📂 Showing the definition (first match)..." DEF_FILE=$(rg -n "teggCtxLifecycleMiddleware" . | head -1 | cut -d: -f1) sed -n '1,200p' "$DEF_FILE"Length of output: 4339
Let’s locate the actual
teggCtxLifecycleMiddlewarefactory implementation to confirm whether it retains any state across invocations.
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Searching for the middleware definition outside of app.ts and controller files..." rg -n "teggCtxLifecycleMiddleware" -g "*.ts" | grep -v "plugin/controller" | grep -v "plugin/tegg/app.ts" echo echo "📂 Showing the first candidate definition..." FILE=$(rg -n "teggCtxLifecycleMiddleware" -g "*.ts" | grep -v "plugin/controller" | grep -v "plugin/tegg/app.ts" | head -1 | cut -d: -f1) sed -n '1,200p' "$FILE"Length of output: 2400
Let’s inspect the
ctx_lifecycle_middlewareimplementation to see if it holds any per-call state:
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Locating ctx_lifecycle_middleware definition..." FILE=$(rg -l "ctx_lifecycle_middleware" -g "*.ts" | head -1) echo "📂 Found in $FILE" sed -n '1,200p' "$FILE"Length of output: 607
I need to inspect the actual
ctx_lifecycle_middlewareimplementation to see if it captures any per‐instance state that would leak across concurrent messages.
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Listing plugin/tegg/app/lib directory:" ls plugin/tegg/app/lib echo echo "🔍 Showing ctx_lifecycle_middleware.ts content (first 200 lines):" sed -n '1,200p' plugin/tegg/app/lib/ctx_lifecycle_middleware.tsLength of output: 193
I need to inspect the actual middleware implementation in
plugin/tegg/libto confirm whether it holds any per-instance state:
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Listing plugin/tegg/lib directory..." ls plugin/tegg/lib echo echo "🔍 Displaying ctx_lifecycle_middleware.ts (first 200 lines)..." sed -n '1,200p' plugin/tegg/lib/ctx_lifecycle_middleware.tsLength of output: 2014
mw is pure and stateless – no change needed
The factory
teggCtxLifecycleMiddleware()simply returns the samectxLifecycleMiddlewarefunction, which allocates its ownlifecycleobject andEggContextImplon each invocation. Combined withctxStorage.run(...), each SSE message runs in its own isolated context storage, so there is no risk of state leaking between concurrent messages. Re-instantiating the middleware insideonmessagedoes not improve safety and can be omitted.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/controller/test/mcp/mcp.test.ts (1)
279-281: Added log verification to test streamable functionalityThis is a good addition that verifies the controller's notification stream completes successfully by checking for the presence of the "startNotificationStream finish" log message. This test enhancement ensures that the teggCtxLifecycleMiddleware properly manages the context lifecycle throughout the request processing flow.
Consider adding a similar verification to the "stateless streamable should work" test for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts(3 hunks)plugin/controller/test/fixtures/apps/mcp-app/package.json(1 hunks)plugin/controller/test/mcp/mcp.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugin/controller/test/fixtures/apps/mcp-app/package.json
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (4)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts (3)
9-11: Updated imports to support loggingThe addition of
LoggerandInjectimports is appropriate for implementing logging functionality in the controller.
27-29: Added dependency injection for LoggerGood implementation of dependency injection for the Logger instance. This follows the framework's patterns and allows for proper logging throughout the controller.
57-58: Added completion logging for notification streamAdding a log message at the completion of the notification stream is valuable for debugging and validates that the method's execution flow completes successfully. This will help verify that the teggCtxLifecycleMiddleware is functioning properly.
plugin/controller/test/mcp/mcp.test.ts (1)
3-3: Added fs/promises import for log verificationThe addition of the fs/promises import is appropriate for the asynchronous file reading needed to verify log output.
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit