Conversation
## Walkthrough
The changes introduce an optional `timeout` property to various MCP-related parameter interfaces and propagate its handling throughout the decorator, metadata, and utility layers. Supporting methods for storing and retrieving controller parameters were added, and related tests and fixtures were updated to verify the new property.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------|
| core/types/controller-decorator/MCPController.ts<br>core/types/controller-decorator/MCPPromptParams.ts<br>core/types/controller-decorator/MCPResourceParams.ts<br>core/types/controller-decorator/MCPToolParams.ts | Added optional `timeout?: number` property to several MCP parameter interfaces; added `title?: string` to MCPPromptParams. |
| core/types/controller-decorator/MetadataKey.ts | Added exported constant symbol `CONTROLLER_MCP_CONTROLLER_PARAMS_MAP`. |
| core/controller-decorator/src/util/MCPInfoUtil.ts | Added static methods to set/get MCP controller params metadata on classes. |
| core/controller-decorator/src/decorator/mcp/MCPController.ts | Decorator now stores the full params object using `MCPInfoUtil.setMCPControllerParams`. |
| core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts | Passes MCP controller params from metadata to `MCPControllerMeta` during build. |
| core/controller-decorator/src/model/MCPControllerMeta.ts | Added optional readonly `timeout` property and constructor parameter for MCP controller params. |
| core/controller-decorator/src/model/MCPPromptMeta.ts | Added optional readonly `title` property and constructor parameter for MCP prompt metadata. |
| core/controller-decorator/test/fixtures/MCPController.ts | Decorator usage updated to include `timeout: 60000` in example controller. |
| core/controller-decorator/test/MCPMeta.test.ts | Test extended to assert the `timeout` property on controller metadata. |
| plugin/controller/test/mcp/helper.test.ts | Updated tests to include new `description` and `title` properties in MCPPromptMeta assertions. |
| plugin/controller/lib/impl/mcp/MCPServerHelper.ts | Refactored resource, tool, and prompt registration methods to use unified registration calls with options objects. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant MCPController (Decorator)
participant MCPInfoUtil
participant MCPControllerMetaBuilder
participant MCPControllerMeta
User->>MCPController: Apply decorator with params (including timeout)
MCPController->>MCPInfoUtil: setMCPControllerParams(params, constructor)
MCPController-->>User: Decorated class
MCPControllerMetaBuilder->>MCPInfoUtil: getMCPControllerParams(clazz)
MCPInfoUtil-->>MCPControllerMetaBuilder: params (possibly with timeout)
MCPControllerMetaBuilder->>MCPControllerMeta: Construct with params (timeout)
MCPControllerMeta-->>MCPControllerMetaBuilder: MCPControllerMeta instance with timeoutPossibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @akitaSummer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new timeout configuration option across various Model Context Protocol (MCP) decorators. The changes involve extending existing type definitions, establishing a robust metadata storage and retrieval mechanism for controller parameters, and ensuring the timeout value is correctly integrated into the controller's metadata for subsequent use. A new test case has also been added to validate this functionality.
Highlights
- New Feature: Timeout Configuration: I've introduced a
timeoutproperty that can now be specified within various Model Context Protocol (MCP) related decorators, including@MCPController,@MCPPrompt,@MCPResource, and@MCPTool. This allows for setting specific timeout durations for these components. - Metadata Management Enhancement: To support the new
timeoutfeature, I've implemented a mechanism to store and retrieveMCPControllerParamsas metadata. This involves defining a new metadata key (CONTROLLER_MCP_CONTROLLER_PARAMS_MAP) and adding utility methods inMCPInfoUtilto manage this data. - Data Flow Integration: The
timeoutvalue provided in the@MCPControllerdecorator is now correctly propagated through theMCPControllerMetaBuilderand stored within theMCPControllerMetaobject. This ensures the configuration is accessible where needed. - Type Definition Updates: I've updated the relevant TypeScript interfaces (
MCPControllerParams,MCPPromptParams,MCPResourceUriParams,MCPResourceTemplateParams,MCPToolParams) to include the new optionaltimeout?: numberproperty, ensuring type safety and clarity for developers.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a timeout feature for MCP controllers by adding a timeout property to the parameters of MCPController, MCPPrompt, MCPTool, and MCPResource decorators. While the configuration for the controller-level timeout is correctly stored in the controller's metadata, the core logic to enforce this timeout appears to be missing, rendering the feature non-functional. Additionally, the implementation for method-level timeouts (for prompts, tools, and resources) is incomplete, as the timeout values are defined in interfaces but not yet utilized. Feedback is provided to address the missing timeout enforcement logic and to improve test coverage to validate the feature's functionality.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
core/controller-decorator/test/MCPMeta.test.ts (1)
24-24: Consider enhancing test coverage for timeout functionality.The basic assertion is good, but consider adding additional test cases to cover:
- Default behavior when timeout is not specified
- Different timeout values
- Edge cases (zero, negative values)
assert(fooControllerMetaData.timeout === 60000); + +// Additional test cases for timeout +it('should handle missing timeout', () => { + // Test controller without timeout specified +}); + +it('should handle different timeout values', () => { + // Test various timeout configurations +});core/types/controller-decorator/MCPToolParams.ts (1)
13-13: Add documentation for the timeout property.The timeout property addition is good, but consider adding JSDoc comments to clarify its purpose and units.
export interface MCPToolParams { name?: string; description?: string; + /** Timeout in milliseconds for tool execution */ timeout?: number; }core/types/controller-decorator/MCPPromptParams.ts (1)
13-13: Add documentation for consistency with other interfaces.The timeout property follows the same pattern as other interfaces. Consider adding JSDoc documentation to match the suggestion for MCPToolParams.
export interface MCPPromptParams { name?: string; description?: string; + /** Timeout in milliseconds for prompt execution */ timeout?: number; }core/types/controller-decorator/MCPController.ts (1)
6-6: Consider validation and documentation for the controller timeout.The timeout property addition is consistent with other interfaces. Since this is the main controller interface, consider:
- Adding JSDoc documentation
- Validation for reasonable timeout values in the implementation
export interface MCPControllerParams { protoName?: string; controllerName?: string; name?: string; version?: string; + /** Timeout in milliseconds for controller operations */ timeout?: number; }Also consider implementing validation in the decorator to ensure timeout values are reasonable (positive numbers, reasonable upper bounds).
core/controller-decorator/src/model/MCPControllerMeta.ts (1)
39-39: Consider potential parameter redundancy.The constructor now accepts both individual parameters (like
name,version) and ametaobject that could contain the same properties. While this maintains backward compatibility, it could lead to confusion if both are provided.Consider documenting the precedence or adding validation to ensure consistency between individual parameters and the meta object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
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/MCPControllerMeta.ts(4 hunks)core/controller-decorator/src/util/MCPInfoUtil.ts(2 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)
🧰 Additional context used
🧬 Code Graph Analysis (4)
core/controller-decorator/src/decorator/mcp/MCPController.ts (1)
core/controller-decorator/src/util/MCPInfoUtil.ts (1)
MCPInfoUtil(42-166)
core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts (2)
core/controller-decorator/src/util/MCPInfoUtil.ts (1)
MCPInfoUtil(42-166)core/controller-decorator/src/model/MCPControllerMeta.ts (1)
MCPControllerMeta(7-73)
core/controller-decorator/src/util/MCPInfoUtil.ts (4)
core/types/controller-decorator/MCPController.ts (1)
MCPControllerParams(1-7)core/types/core-decorator/model/EggPrototypeInfo.ts (1)
EggProtoImplClass(5-5)core/core-decorator/src/util/MetadataUtil.ts (1)
MetadataUtil(5-97)core/types/controller-decorator/MetadataKey.ts (1)
CONTROLLER_MCP_CONTROLLER_PARAMS_MAP(29-29)
core/controller-decorator/src/model/MCPControllerMeta.ts (1)
core/types/controller-decorator/MCPController.ts (1)
MCPControllerParams(1-7)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (20)
🔇 Additional comments (12)
core/types/controller-decorator/MetadataKey.ts (1)
29-29: LGTM: Metadata key follows established patterns.The new metadata key is correctly implemented following the same naming convention and pattern as other metadata keys in the file.
core/controller-decorator/test/fixtures/MCPController.ts (1)
21-21: LGTM: Timeout property correctly added to test fixture.The timeout value of 60000ms (60 seconds) is appropriate for testing the new timeout functionality.
core/types/controller-decorator/MCPResourceParams.ts (2)
8-8: LGTM: Timeout property correctly added to MCPResourceUriParams.The optional timeout property is properly typed and consistent with the broader timeout feature implementation.
15-15: LGTM: Timeout property correctly added to MCPResourceTemplateParams.The optional timeout property maintains consistency with the MCPResourceUriParams interface.
core/controller-decorator/src/decorator/mcp/MCPController.ts (1)
32-32: LGTM: Parameter storage correctly added to preserve timeout property.The call to
setMCPControllerParamsproperly stores the entire parameter object, ensuring the timeout property is preserved along with other controller parameters.core/controller-decorator/src/impl/mcp/MCPControllerMetaBuilder.ts (2)
76-76: LGTM: Controller parameters correctly retrieved for metadata construction.The retrieval of MCP controller parameters using
MCPInfoUtil.getMCPControllerParamsfollows the established pattern and enables access to the timeout property.
79-79: LGTM: Metadata parameter correctly passed to constructor.The
metaparameter is properly passed to theMCPControllerMetaconstructor, allowing the timeout property to be initialized from the stored controller parameters.core/controller-decorator/src/util/MCPInfoUtil.ts (2)
17-18: LGTM: Required imports correctly added.The imports for
MCPControllerParamsandCONTROLLER_MCP_CONTROLLER_PARAMS_MAPare necessary for the new controller parameter management functionality.
60-66: LGTM: Controller parameter management methods correctly implemented.The new
setMCPControllerParamsandgetMCPControllerParamsmethods follow the established pattern of other metadata management methods in the class. They properly handle theMCPControllerParamstype and use the correct metadata key for storage and retrieval.core/controller-decorator/src/model/MCPControllerMeta.ts (3)
2-2: LGTM!The import addition is necessary for the new constructor parameter type.
21-21: LGTM!The timeout property follows the established pattern of being readonly and optional, consistent with other properties in the class.
53-53: LGTM!The timeout assignment correctly uses optional chaining to handle the case where
metais undefined, following good TypeScript practices.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
plugin/controller/test/mcp/helper.test.ts (1)
48-58: Consider adding timeout test coverage.While the PR is titled "feat: add timeout" and the AI summary mentions timeout functionality being added to interfaces, this test doesn't verify timeout behavior. Consider adding test cases to ensure the timeout property is properly handled.
Would you like me to help generate additional test cases to cover the timeout functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/controller-decorator/src/model/MCPPromptMeta.ts(3 hunks)core/types/controller-decorator/MCPPromptParams.ts(1 hunks)plugin/controller/test/mcp/helper.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/controller-decorator/src/model/MCPPromptMeta.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- core/types/controller-decorator/MCPPromptParams.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
plugin/controller/test/mcp/helper.test.ts (1)
52-53: LGTM: New properties added to test setup.The addition of
descriptionandtitleproperties to thepromptMetaconfiguration is consistent with the interface changes mentioned in the PR summary.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
plugin/controller/lib/impl/mcp/MCPServerHelper.ts (1)
1-12: Timeout Support in MCP Registration APIs Requires Manual ImplementationThe MCP SDK’s
registerResource,registerTool, andregisterPromptmethods do not accept a timeout option in their signatures. By default, the SDK enforces a protocol-level timeout (30–60 s), which can only be overridden via server configuration or anoptionsparameter when handling requests—not at registration time.To satisfy the PR’s intent of adding timeouts, please update your handlers in
plugin/controller/lib/impl/mcp/MCPServerHelper.tsto enforce timeouts manually or via server config:• Wrap each callback in a
Promise.raceagainst a timeout promise:server.registerResource(name, async (resource) => { return Promise.race([ userReadCallback(resource), new Promise((_, reject) => setTimeout(() => reject(new Error('Resource request timed out')), 5000) ), ]); });• Or instantiate/configure
McpServerwith a custom timeout if your SDK version supports anoptions.timeoutoverride when creating the server or handling calls.Locations to update:
- plugin/controller/lib/impl/mcp/MCPServerHelper.ts – implementations of
registerResource,registerTool, andregisterPromptEnsure you’re on the latest
@modelcontextprotocol/sdkrelease for any built-in timeout overrides; otherwise, the manual wrapping approach is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin/controller/lib/impl/mcp/MCPServerHelper.ts(3 hunks)plugin/controller/test/mcp/helper.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/test/mcp/helper.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (1)
plugin/controller/lib/impl/mcp/MCPServerHelper.ts (1)
53-57: Ensure timeout is propagated in resource registrationThe PR adds a
timeoutproperty in metadata but the call tothis.server.registerResourceisn’t including it. Please verify thatMCPResourceMetadefinestimeoutand thatregisterResourceaccepts it, then update as follows:
- File: plugin/controller/lib/impl/mcp/MCPServerHelper.ts
- Lines: around 53–57
Suggested change:
- if (resourceMeta.uri || resourceMeta.template) { - this.server.registerResource( - name, - resourceMeta.uri ?? resourceMeta.template!, - resourceMeta.metadata ?? {}, - handler - ); + if (resourceMeta.uri || resourceMeta.template) { + const metadata = { + ...(resourceMeta.metadata ?? {}), + ...(resourceMeta.timeout != null ? { timeout: resourceMeta.timeout } : {}), + }; + this.server.registerResource( + name, + resourceMeta.uri ?? resourceMeta.template!, + metadata, + handler + ); } else { throw new Error(`MCPResource ${name} must have uri or template`); }
2 participants
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Bug Fixes
Tests