Conversation
martin-fleck-at
left a comment
martin-fleck-at
left a comment
There was a problem hiding this comment.
Hi @Sakrafux! Thank you for the great overall work! I do have some comments and questions regarding parts of the code but I am a big fan of the overall ideas and configurability. Since we are not merging into main (yet), this definitely does not need to be perfect but I would love to get your thoughts and feedback on some of the comments.
|
|
||
| protected override builder(point?: Point): ActivityNodeBuilder { | ||
| return super.builder(point).addCssClass('decision').resizeLocations(GResizeLocation.CROSS); | ||
| return super.builder(point).addCssClass('decision').resizeLocations(GResizeLocation.CROSS).size(32, 32); |
There was a problem hiding this comment.
Why is necessary to hard-code the size here if it was not necessary before? (same in the merge-node-handler)
| result = await this.initializeServer(params, result); | ||
| // This server is generated as response on diagram request, | ||
| // i.e., the WebSocketServerLauncher starts DefaultGLSPServer per WS connection and this starts the McpServerManager | ||
| // For the simple browser "client", this means every client has a new GLSP Server and MCP Server, but this does not generally hold |
There was a problem hiding this comment.
Could you elaborate on that a bit? What exactly does not hold?
Since this is the generic GLSP server, I don't think it should need to know about MCP, so the initialzation of the mcpServer parameter should not be done here.
| const element = this.createNode(operation, relativeLocation); | ||
| if (element) { | ||
| // When handling IDs that are not guaranteed unique, ensure no collisions | ||
| // Since this is a constant time access, the performance impact should be negligable |
There was a problem hiding this comment.
In what scenarios did we generate duplicate ids? I'm slightly torn between adapting the id and simply throwing an error to have that clean up as adopters might depend on specific IDs.
|
|
||
| const AGENT_PERSONA = ` | ||
| You are the GLSP Modeling Agent. Your primary goal is to assist in the creation and modification of graphical models using the | ||
| CLSP MCP server. You have to adhere to the following principles: |
There was a problem hiding this comment.
| CLSP MCP server. You have to adhere to the following principles: | |
| GLSP MCP server. You have to adhere to the following principles: |
| options: McpServerOptions; | ||
| } | ||
|
|
||
| const AGENT_PERSONA = ` |
There was a problem hiding this comment.
I think it would be nice to have that as part of the options as well.
| if (element) { | ||
| realIds.push(realId); | ||
| } else { | ||
| missingIds.push(elementId); |
There was a problem hiding this comment.
Should we push the elementId or realId here? We expose it as part of the error message so we need to make sure that we consistently use either the alias IDs or the real IDs, depending on who is supposed to deal with the message (human vs LLM).
Note: I think overall this raises a very interesting question because tool functions are not only called and evaluated by the LLM, a user may also trigger a tool (e.g., delete the node with id hello) or may need to understand the result. So when we determine the element id, we should always try to use the given id first and only then try to see if it is an alias. This adds some complexity but at least makes sure that both set of ids are usable, what do you think?
| ); | ||
| // A connection may be unceremoniously be closed (e.g., closing/reloading the browser) in which | ||
| // case the server must still be disposed | ||
| serverInstance.clientConnection.onClose(() => this.disposeServerInstance(serverInstance)); |
There was a problem hiding this comment.
Have you checked whether this accounts for the reconnect behavior? But overall this looks good.
| if (options.webSocket) { | ||
| const launcher = appContainer.resolve(WebSocketServerLauncher); | ||
| launcher.configure(serverModule, mcpModule); | ||
| launcher.configure(serverModule, mcpModule, configureWorfklowMcpModule()); |
There was a problem hiding this comment.
nit: I think the API can be improved here but it does not need to happen as part of this PR.
| @inject(ClientSessionManager) | ||
| protected clientSessionManager: ClientSessionManager; | ||
|
|
||
| protected resolvers: Record<string, { sessionId: string; resolve: (value: CallToolResult | PromiseLike<CallToolResult>) => void }> = {}; |
There was a problem hiding this comment.
I'm not a big fan of this but until eclipse-glsp/glsp#607 is resolved, there really is not good alternative. We should definitely tackle that next.
| const mcpServerConfig: FullMcpServerConfiguration = { port, host, route, name }; | ||
| // use a fixed default port instead of 0 so that the MCP server need only be registered once | ||
| // using 0, i.e., a random port, would require re-registering the MCP server each time | ||
| const { port = 60000, host = '127.0.0.1', route = '/glsp-mcp', name = 'glspMcpServer', options = {} } = mcpServerParam; |
There was a problem hiding this comment.
I assume this is only set like this for testing?
2 participants
Extends the GLSP MCP Server started in PR-120 according to our meetings @martin-fleck-at.