Skip to content

feat(mcp): enhance server management with resource handling and trans… #15413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

kaikongbj
Copy link

@kaikongbj kaikongbj commented Apr 7, 2025

This pull request includes several updates and improvements to the MCP (Model Context Protocol) integration within the Theia framework. The changes primarily focus on enhancing the MCP server management capabilities, updating dependencies, and improving error handling.

Dependency Updates:

  • Updated the dependency version for @modelcontextprotocol/sdk in package.json to use a caret (^) to allow for minor version updates.

Error Handling Improvements:

  • Modified various methods in MCPCommandContribution to use await with messageService calls for better error handling and user feedback. [1] [2] [3] [4]

MCP Server Enhancements:

  • Added new methods listResources and readResource to the MCPServer and MCPServerManager interfaces to support resource listing and reading functionalities. [1] [2] [3] [4]
  • Introduced support for SSE (Server-Sent Events) transport in MCPServer with corresponding logic to handle different transport types (stdio and sse). [1] [2] [3] [4]

Code Refactoring:

  • Simplified import statements in mcp-command-contribution.ts by using a more concise path for MCPFrontendService and MCPServerStatus.
  • Improved error handling in MCPServer by specifying the error type as Error and handling unknown errors gracefully.

setup example

"ai-features.mcp.mcpServers": {
"plugin": {
"args": [],
"autostart": false,
"command": "plugin",
"env": {
"sseUrl": "http://127.0.0.1:13532/sse"
},
"name": "plugin"
}"

@github-project-automation github-project-automation bot moved this to Waiting on reviewers in PR Backlog Apr 7, 2025
@JonasHelming
Copy link
Contributor

Thank you for the contribution!
Great that you have signed the ECA!

I left some comments on the closed variant of this (#15233):

  1. Could you add information on how to test this? I.e. how to set up a (ideally standard node-based) MCP server this way.
    Does it also make sense to allow the user to configure such servers or should this be an "API-only" feature?

  2. Having the transport Type and the sseUrl as separate variables is maybe not optimal, if you use SSE as a transport type, the url is mandatory. As we currently support two transport types, I would either
    a) Only provide the seeURL and if it is set we use SSE
    b) Include the transport specific parameters (env and URL) into a transportType object (with two versions)

The second options seems cleaner to me, as env is ignored for SSE. This way, the API makes it very clear what parameters you can provide

@@ -3,7 +3,7 @@
"version": "1.60.0",
"description": "Theia - MCP Integration",
"dependencies": {
"@modelcontextprotocol/sdk": "1.0.1",
"@modelcontextprotocol/sdk": "^1.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"@modelcontextprotocol/sdk": "^1.0.1"
"@modelcontextprotocol/sdk": "^1.0.1",

This breaks the build

env: mergedEnv,
});
transport.onerror = error => {
console.log(`Starting server "${this.name}" with transport: ${this.transportType}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We log two messages before we start a server now. I think we can combine it to one covering both cases. What I mean: I would merge the content of the two messages below into the message here so that we get only one log message including all information (the URL for SSE, the command, args and env for stdio)

@github-project-automation github-project-automation bot moved this from Waiting on reviewers to Waiting on author in PR Backlog Apr 16, 2025
this.transportType = 'sse';
} else {
this.transportType = 'stdio';
}
console.log(this.autostart);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(this.autostart);

This was there before, but we could remove it.

@JonasHelming
Copy link
Contributor

JonasHelming commented Apr 16, 2025

Thank you for iterating on the changes!

Question: Is there any publicly available MCP server supporting SSE we can use to test this? The example you provide just refers to "plugin".

Comment: You moved the sseURL to an environment variable now, which does not seem like a good fit. If you use SSE, you do not need command, args, env. So I would be in favor of making this explicit.

The user preferences could look like below then (mcp-preferences.ts). The interface MCPServerDescription (in mcp-server-manager.ts) could have two sub types then for the two variants. They are read from the preferences in mcp-forntend-application-contribution.ts

This would make the user configuration and the handling of the two differen types of severs pretty clean I believe, WDYT?

additionalProperties: {
        oneOf: [
          {
            type: 'object',
            properties: {
              command: {
                type: 'string',
                title: nls.localize('theia/ai/mcp/servers/command/title', 'Command to execute the MCP server'),
                markdownDescription: nls.localize('theia/ai/mcp/servers/command/mdDescription', 'The command used to start the MCP server, e.g., "uvx" or "npx".')
              },
              args: {
                type: 'array',
                title: nls.localize('theia/ai/mcp/servers/args/title', 'Arguments for the command'),
                markdownDescription: nls.localize('theia/ai/mcp/servers/args/mdDescription', 'An array of arguments to pass to the command.'),
              },
              env: {
                type: 'object',
                title: nls.localize('theia/ai/mcp/servers/env/title', 'Environment variables'),
                markdownDescription: nls.localize('theia/ai/mcp/servers/env/mdDescription', 'Optional environment variables to set for the server, such as an API key.'),
                additionalProperties: {
                  type: 'string'
                }
              },
              autostart: {
                type: 'boolean',
                title: nls.localize('theia/ai/mcp/servers/autostart/title', 'Autostart'),
                markdownDescription: nls.localize('theia/ai/mcp/servers/autostart/mdDescription',
                  'Automatically start this server when the frontend starts. Newly added servers are not immediately auto started, but on restart'),
                default: true
              }
            },
            required: ['command', 'args']
          },
          {
            type: 'object',
            properties: {
              sseUrl: {
                type: 'string',
                title: nls.localize('theia/ai/mcp/servers/sseUrl/title', 'Server-Sent Events URL'),
                markdownDescription: nls.localize('theia/ai/mcp/servers/sseUrl/mdDescription', 'The URL for the server-sent events from the remote MCP server.')
              },
              autostart: {
                type: 'boolean',
                title: nls.localize('theia/ai/mcp/servers/autostart/title', 'Autostart'),
                markdownDescription: nls.localize('theia/ai/mcp/servers/autostart/mdDescription',
                  'Automatically connect to this server when the frontend starts. Newly added servers are not immediately auto started, but on restart'),
                default: true
              }
            },
            required: ['sseUrl']
          }
        ]
      }

@fipro78
Copy link
Contributor

fipro78 commented May 14, 2025

We tested this contribution locally by rebinding a custom implementation that mainly follows this PR with the following additions:

  • Updated "@modelcontextprotocol/sdk": "^1.11.0" to support sse authentication
  • Added sseAuthToken to the env to verify that authentication works

We were able to verify that it basically works. How could we help to proceed with getting this PR included so that MCP in Theia supports SSE servers?

We are also unsure whether the configuration in the env makes sense, or if the sse configurations should be on top level.
We also noticed that the command configuration is required in the Theia code, but with an SSE server it is not necessary. Could it be optional then?

@JonasHelming
Copy link
Contributor

Hi @fipro78 , if you do not mind, please go ahead and open a new PR with your changes, as we did not get any response to the comments on this PR.
Maybe it makes sense to introduce two different types for the user settings (local and remote) as the properties are mainly not overlapping. It also makes sense to not create a command for the remote ones as you stated.

@fipro78
Copy link
Contributor

fipro78 commented Jun 6, 2025

@JonasHelming
Short update, we are working on a PR. We noticed that SSE is already deprecated for MCP and replaced with StreamableHttp. We try to cover both scenarios and prepare a new PR.

Just to keep you updated on the topic.

@JonasHelming
Copy link
Contributor

@fipro78 Thank you, this sounds great!

@kaikongbj kaikongbj closed this Jun 29, 2025
@github-project-automation github-project-automation bot moved this from Waiting on author to Done in PR Backlog Jun 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants