From 7773dd6b946aa6b50fb3d54f5e9d3cd049dc1e99 Mon Sep 17 00:00:00 2001 From: Martin Fleck Date: Thu, 27 Aug 2026 11:45:16 +0200 Subject: [PATCH] fix(server-mcp): correct tool result contracts and registration Result-contract fixes - Emit dispatchedCommands from the create-edges dry run so the SDK stops replacing the verdicts with an output-validation error - Bind outputSchema and the success() payload to a shared O generic, so a handler declaring one shape and emitting another no longer compiles - Report actual undo/redo counts by re-checking the command stack per iteration instead of echoing the requested count - Count echoed identities rather than inputs when reporting how many nodes and edges were modified Behaviour fixes - Save to an explicit fileUri even when the command stack is clean, so save-as no longer no-ops - Reject bounds changes on non-node elements and any change targeting the diagram root, both of which core silently drops or misapplies - Surface an error for node and edge entries that request no change rather than counting them as modified - Throw on unknown ids in validate-diagram and set-view instead of reporting an empty, clean-looking result - Fall back to the first new element when the created type differs from the requested elementTypeId Registration gating - Add isSupportedByDiagramType(), evaluated per diagram type at harvest, and drop unsupported handlers before they reach the MCP catalog - Gate layout on it so it is no longer advertised without a LayoutEngine - Probe only handlers that declare the hook, keeping the harvest clear of unrelated @postConstruct side effects - Keep canRegister() for connected-client capability, which the harvest container cannot answer Schema and doc fixes - Add a shared elementIdsAllowingEmpty fragment for set-selection, and require integer undo and redo counts - Describe modify-nodes positions as parent-relative and create-nodes positions as absolute - Write the validate-diagram dedup separator as an escape rather than raw NUL bytes, which had git classifying the source file as binary --- CHANGELOG.md | 16 ++++ .../src/common/modules/mcp-diagram-module.ts | 2 +- .../mcp-diagram-handler-dispatcher.spec.ts | 90 ++++++++++++++++++ .../server/mcp-diagram-handler-dispatcher.ts | 61 +++++++++++- .../src/common/server/mcp-input-schemas.ts | 3 + .../src/common/server/mcp-resource-handler.ts | 14 ++- .../common/server/mcp-tool-handler.spec.ts | 2 +- .../src/common/server/mcp-tool-handler.ts | 66 +++++++++---- .../count-elements-mcp-tool-handler.ts | 3 +- .../create-edges-mcp-tool-handler.spec.ts | 2 + .../handlers/create-edges-mcp-tool-handler.ts | 28 ++++-- .../create-nodes-mcp-tool-handler.spec.ts | 2 + .../handlers/create-nodes-mcp-tool-handler.ts | 27 ++++-- .../delete-elements-mcp-tool-handler.ts | 3 +- .../diagram-model-mcp-tool-handler.ts | 9 +- .../element-types-mcp-tool-handler.ts | 9 +- .../get-selection-mcp-tool-handler.ts | 3 +- .../tools/handlers/layout-mcp-tool-handler.ts | 10 +- .../handlers/modify-edges-mcp-tool-handler.ts | 22 +++-- .../handlers/modify-nodes-mcp-tool-handler.ts | 88 ++++++++++++----- .../query-elements-mcp-tool-handler.spec.ts | 3 + .../query-elements-mcp-tool-handler.ts | 3 +- .../tools/handlers/redo-mcp-tool-handler.ts | 13 ++- .../handlers/save-model-mcp-tool-handler.ts | 7 +- .../session-info-mcp-tool-handler.spec.ts | 2 + .../handlers/session-info-mcp-tool-handler.ts | 3 +- .../set-selection-mcp-tool-handler.spec.ts | 27 +++++- .../set-selection-mcp-tool-handler.ts | 13 ++- .../handlers/set-view-mcp-tool-handler.ts | 5 +- .../test/expect-structured-content.ts | 34 +++++++ .../tools/handlers/undo-mcp-tool-handler.ts | 13 ++- .../validate-diagram-mcp-tool-handler.ts | Bin 5739 -> 5843 bytes 32 files changed, 475 insertions(+), 108 deletions(-) create mode 100644 packages/server-mcp/src/common/tools/handlers/test/expect-structured-content.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 34c56b4..de54a3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ - the web socket event carries the upgrade request next to the socket, which is the only place its headers and query are still available - [launch] Release the server socket again when a restarted launcher is shut down [#150](https://github.com/eclipse-glsp/glsp-server-node/pull/150) - `WebSocketServerLauncher` now also closes the HTTP server it mounts on, which `ws` leaves listening because it did not create it +- [mcp] Return results that satisfy the declared output schema, so a `create-edges` dry run returns its verdicts instead of an output-validation error [#152](https://github.com/eclipse-glsp/glsp-server-node/pull/152) +- [mcp] Stop reporting success for work that was not done [#152](https://github.com/eclipse-glsp/glsp-server-node/pull/152) + - `save-model` writes to an explicit `fileUri` even when the command stack is clean, instead of skipping a save-as + - `undo` and `redo` report how many commands they applied, not how many were requested + - `modify-nodes` and `modify-edges` report an error for entries that request no change, instead of counting them as modified +- [mcp] Reject unknown element ids in `validate-diagram` and `set-view`, which previously dropped them and returned an empty, clean-looking result [#152](https://github.com/eclipse-glsp/glsp-server-node/pull/152) +- [mcp] Keep tools out of the MCP catalog when no diagram type supports them, so `layout` is no longer advertised without a bound `LayoutEngine` [#152](https://github.com/eclipse-glsp/glsp-server-node/pull/152) + - the new `isSupportedByDiagramType()` hook on the diagram tool and resource bases covers statically bound dependencies; `canRegister()` keeps gating capabilities of the connected GLSP client +- [mcp] Align tool schemas and descriptions with what the tools actually accept and apply [#152](https://github.com/eclipse-glsp/glsp-server-node/pull/152) + - `set-selection` accepts the documented empty-array form for clearing the selection, and `undo` / `redo` require integer counts + - `modify-nodes` positions are parent-relative and `create-nodes` positions absolute, matching the dispatched operations + - the `create-*` tools echo the created element when its type differs from the requested `elementTypeId`, instead of reporting a creation failure ### Potentially Breaking Changes @@ -17,6 +29,10 @@ - `applyElementAndBounds`, `applyAlignment` and `applyRoute` no longer throw for an element the index cannot resolve, they report it as not applied. `applyRoutingPoints` stays strict. - [launch] Launchers register what `shutdown` has to release in the new `GLSPServerLauncher.registerDisposables` hook, called once per launch, rather than in their constructor [#150](https://github.com/eclipse-glsp/glsp-server-node/pull/150) - A custom launcher that pushes into `toDispose` from its constructor keeps compiling but loses that cleanup after the first `shutdown`, because `dispose` empties the collection. Move those registrations into an override of `registerDisposables`. +- [mcp] The MCP tool handler bases take an optional output type parameter, e.g. `AbstractMcpDiagramToolHandler`, bound to the handler's declared `outputSchema` [#152](https://github.com/eclipse-glsp/glsp-server-node/pull/152) + - The parameter defaults, so a handler without an `outputSchema` is unaffected. A subclass that passes `success()` a payload not matching the overridden handler's output schema now fails to compile, instead of producing an error result at call time. +- [mcp] `modify-nodes` rejects `position` / `size` for elements that are not a `GNode`, which core's bounds handler silently ignored while the tool reported success [#152](https://github.com/eclipse-glsp/glsp-server-node/pull/152) + - Adopters who bind a bounds handler covering more element kinds override the guard in `ModifyNodesMcpToolHandler`. ## [v2.7.0 - 01/06/2026](https://github.com/eclipse-glsp/glsp-server-node/releases/tag/v2.7.0) diff --git a/packages/server-mcp/src/common/modules/mcp-diagram-module.ts b/packages/server-mcp/src/common/modules/mcp-diagram-module.ts index 6fbb969..c4b3f74 100644 --- a/packages/server-mcp/src/common/modules/mcp-diagram-module.ts +++ b/packages/server-mcp/src/common/modules/mcp-diagram-module.ts @@ -215,7 +215,7 @@ export abstract class AbstractMcpDiagramModule extends GLSPModule { binding.add(GetSelectionMcpToolHandler); binding.add(SetSelectionMcpToolHandler); binding.add(SetViewMcpToolHandler); - // Auto-skips at session-open via `canRegister()` when no `LayoutEngine` is bound. + // Auto-skips via `isSupportedByDiagramType()` when no `LayoutEngine` is bound. binding.add(LayoutMcpToolHandler); } diff --git a/packages/server-mcp/src/common/server/mcp-diagram-handler-dispatcher.spec.ts b/packages/server-mcp/src/common/server/mcp-diagram-handler-dispatcher.spec.ts index bcfa4ab..ebb6f42 100644 --- a/packages/server-mcp/src/common/server/mcp-diagram-handler-dispatcher.spec.ts +++ b/packages/server-mcp/src/common/server/mcp-diagram-handler-dispatcher.spec.ts @@ -14,10 +14,13 @@ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 ********************************************************************************/ +import { Logger, NullLogger } from '@eclipse-glsp/server'; import { CallToolResult, ReadResourceResult } from '@modelcontextprotocol/sdk/types.js'; +import { Container, ContainerModule, inject, injectable, optional } from 'inversify'; import { describe, expect, it } from 'vitest'; import { GLSPMcpServer } from './glsp-mcp-server'; import { DefaultMcpDiagramHandlerDispatcher, DiagramTypeCatalog } from './mcp-diagram-handler-dispatcher'; +import { McpDiagramToolHandlerConstructor } from './mcp-tool-handler'; /** * Tests the SDK-callback dispatch error path covered by `runWithToolErrorEnvelope`. The @@ -112,6 +115,93 @@ class FakeStaticResourceHandlerCtor { } } +/** Stands in for a statically bound, optional dependency such as `LayoutEngine`. */ +const OptionalDependency = Symbol('OptionalDependency'); + +@injectable() +class GatedToolHandler { + @inject(OptionalDependency) @optional() protected dependency?: unknown; + + readonly name = 'gated-tool'; + readonly description = 'Only supported when the optional dependency is bound'; + readonly inputSchema = { shape: {}, strict: () => ({}) }; + + isSupportedByDiagramType(): boolean { + return this.dependency !== undefined; + } + + toRegistrationConfig(): unknown { + return { description: this.description, inputSchema: {} }; + } +} + +/** Never bound, so resolving {@link UnresolvableToolHandler} through the container throws. */ +const UnboundDependency = Symbol('UnboundDependency'); + +/** Declares the hook, so it is probed, but its required dependency has no binding. */ +@injectable() +class UnresolvableToolHandler { + @inject(UnboundDependency) protected dependency: unknown; + + readonly name = 'unresolvable-tool'; + readonly description = 'Declares the support hook but cannot be constructed by the container'; + readonly inputSchema = { shape: {}, strict: () => ({}) }; + + isSupportedByDiagramType(): boolean { + return false; + } + + toRegistrationConfig(): unknown { + return { description: this.description, inputSchema: {} }; + } +} + +function harvestWith(bindDependency: boolean, constructors: unknown[] = [GatedToolHandler]): DefaultMcpDiagramHandlerDispatcher { + const diagramModule = new ContainerModule(bind => { + bind(McpDiagramToolHandlerConstructor).toConstantValue(constructors as McpDiagramToolHandlerConstructor[]); + if (bindDependency) { + bind(OptionalDependency).toConstantValue({}); + } + }); + const dispatcher = new DefaultMcpDiagramHandlerDispatcher(); + (dispatcher as unknown as { serverContainer: Container }).serverContainer = new Container(); + (dispatcher as unknown as { diagramModules: Map }).diagramModules = new Map([['test', [diagramModule]]]); + (dispatcher as unknown as { logger: Logger }).logger = new NullLogger(); + dispatcher.harvest(); + return dispatcher; +} + +describe('DefaultMcpDiagramHandlerDispatcher · diagram-type support gate', () => { + it('registers the tool when the diagram type binds its dependency', () => { + const captured = new CapturingMcpServer(); + harvestWith(true).registerAll(captured as unknown as GLSPMcpServer, false); + + expect(captured.tools.has('gated-tool')).toBe(true); + }); + + it('keeps the tool out of the catalog when no diagram type supports it', () => { + const captured = new CapturingMcpServer(); + harvestWith(false).registerAll(captured as unknown as GLSPMcpServer, false); + + expect(captured.tools.has('gated-tool')).toBe(false); + }); + + it('fails open and registers a handler that declares the hook but cannot be resolved', () => { + const captured = new CapturingMcpServer(); + harvestWith(false, [UnresolvableToolHandler]).registerAll(captured as unknown as GLSPMcpServer, false); + + // `isSupportedByDiagramType` returns false, so it survives only because probing threw. + expect(captured.tools.has('unresolvable-tool')).toBe(true); + }); + + it('registers a duck-typed handler without the hook, without resolving it', () => { + const captured = new CapturingMcpServer(); + harvestWith(false, [FakeToolHandlerCtor]).registerAll(captured as unknown as GLSPMcpServer, false); + + expect(captured.tools.has('fake-tool')).toBe(true); + }); +}); + describe('DefaultMcpDiagramHandlerDispatcher · SDK-callback dispatch error envelope', () => { it('tool callback returns isError envelope when sessionId is missing', async () => { const dispatcher = makeDispatcher({ diff --git a/packages/server-mcp/src/common/server/mcp-diagram-handler-dispatcher.ts b/packages/server-mcp/src/common/server/mcp-diagram-handler-dispatcher.ts index a3d4bca..bf8e920 100644 --- a/packages/server-mcp/src/common/server/mcp-diagram-handler-dispatcher.ts +++ b/packages/server-mcp/src/common/server/mcp-diagram-handler-dispatcher.ts @@ -37,6 +37,11 @@ import { McpRequestContext, NoopMcpRequestContext } from './mcp-request-context' import { AbstractMcpDiagramResourceHandler, McpDiagramResourceHandlerConstructor, toParams } from './mcp-resource-handler'; import { BaseMcpDiagramToolHandler, McpDiagramToolHandlerConstructor } from './mcp-tool-handler'; +/** The diagram-type support hook shared by the diagram-scope tool and resource handler bases. */ +interface DiagramTypeSupportAware { + isSupportedByDiagramType(): boolean; +} + /** * Per-diagram-type catalog of constructor lists, harvested at MCP-server start by loading each * diagram type's modules onto a temporary child container — same pattern as @@ -97,11 +102,13 @@ export class DefaultMcpDiagramHandlerDispatcher implements McpDiagramHandlerDisp /** * Build the per-diagram-type catalog by inspecting each diagram type's module set. We don't - * have a real GLSP session yet — and we don't want one, because we only need the bound + * have a real GLSP session yet — and we don't want one, because we mostly need the bound * constructor *lists*, not instances. So we spin up a throwaway child container per diagram * type, load its modules plus a placeholder session module, and read out the multi-binding - * constants. No handler is instantiated; the temporary container is unbound immediately - * after. + * constants. The temporary container is unbound immediately after. + * + * Only handlers that override `isSupportedByDiagramType` are instantiated, so a handler with + * a `@postConstruct` sees this probe just because it opted into the support gate. * * The placeholder session module is bound with the synthetic {@link TEMPORARY_CLIENT_ID} so * any session-scoped `@inject(ClientId)` in module-load wiring resolves cleanly. Diagram @@ -123,8 +130,18 @@ export class DefaultMcpDiagramHandlerDispatcher implements McpDiagramHandlerDisp for (const [diagramType, modules] of this.diagramModules) { const tempContainer = this.serverContainer.createChild(); tempContainer.load(...modules, placeholderSessionModule); - const tools = getConstructorList(tempContainer, McpDiagramToolHandlerConstructor); - const resources = getConstructorList(tempContainer, McpDiagramResourceHandlerConstructor); + const tools = this.filterSupported( + tempContainer, + getConstructorList(tempContainer, McpDiagramToolHandlerConstructor), + diagramType, + BaseMcpDiagramToolHandler.prototype.isSupportedByDiagramType + ); + const resources = this.filterSupported( + tempContainer, + getConstructorList(tempContainer, McpDiagramResourceHandlerConstructor), + diagramType, + AbstractMcpDiagramResourceHandler.prototype.isSupportedByDiagramType + ); const prompts = getConstructorList(tempContainer, McpDiagramPromptHandlerConstructor); tempContainer.unbindAll(); catalogs.push({ diagramType, toolConstructors: tools, resourceConstructors: resources, promptConstructors: prompts }); @@ -132,6 +149,40 @@ export class DefaultMcpDiagramHandlerDispatcher implements McpDiagramHandlerDisp this.diagramCatalogs = catalogs; } + /** + * Drop constructors whose handler reports the diagram type can't support it, so they never + * reach the MCP catalog. A handler that overrides the hook is resolved against the harvest + * container, where `@optional()` dependencies reflect the diagram type's real bindings; one + * that inherits `defaultHook` or doesn't declare the hook is kept without being instantiated. + * + * Fails open: a constructor that cannot be resolved here stays in the catalog. + */ + protected filterSupported>( + container: Container, + constructors: C[], + diagramType: string, + defaultHook: DiagramTypeSupportAware['isSupportedByDiagramType'] + ): C[] { + return constructors.filter(constructor => { + // A duck-typed handler may not carry the hook at all; skip it rather than resolving it + // only to call a method that isn't there. + const hook = constructor.prototype.isSupportedByDiagramType; + if (typeof hook !== 'function' || hook === defaultHook) { + return true; + } + try { + if (container.resolve(constructor).isSupportedByDiagramType()) { + return true; + } + this.logger.debug(`Diagram type '${diagramType}' does not support MCP handler '${constructor.name}'; not registering.`); + return false; + } catch (err: unknown) { + this.logger.debug(`Could not probe MCP handler '${constructor.name}' for diagram type '${diagramType}'; registering.`, err); + return true; + } + }); + } + /** True when at least one diagram type has at least one tool handler bound. */ hasDiagramTools(): boolean { return this.diagramCatalogs.some(catalog => catalog.toolConstructors.length > 0); diff --git a/packages/server-mcp/src/common/server/mcp-input-schemas.ts b/packages/server-mcp/src/common/server/mcp-input-schemas.ts index 9e2a328..f95b933 100644 --- a/packages/server-mcp/src/common/server/mcp-input-schemas.ts +++ b/packages/server-mcp/src/common/server/mcp-input-schemas.ts @@ -45,6 +45,9 @@ export const elementId = z.string(); /** One or more element ids. Empty arrays are rejected. */ export const elementIds = z.array(z.string()).min(1); +/** Zero or more element ids, for tools where an empty array is itself meaningful (e.g. "select nothing"). */ +export const elementIdsAllowingEmpty = z.array(z.string()); + /** Cartesian position used by node-creation / -modification tools. */ export const position = z .object({ diff --git a/packages/server-mcp/src/common/server/mcp-resource-handler.ts b/packages/server-mcp/src/common/server/mcp-resource-handler.ts index bb8e90d..daced60 100644 --- a/packages/server-mcp/src/common/server/mcp-resource-handler.ts +++ b/packages/server-mcp/src/common/server/mcp-resource-handler.ts @@ -294,7 +294,19 @@ export abstract class AbstractMcpDiagramResourceHandler< return requestActionOrFail(this.actionDispatcher, request, timeoutMs, label); } - /** Override to opt out of registration when a runtime dependency is missing. Default: `true`. */ + /** + * Whether the diagram type can support this resource at all — see + * {@link BaseMcpDiagramToolHandler.isSupportedByDiagramType}. Returning `false` keeps the + * resource out of the MCP catalog. + */ + isSupportedByDiagramType(): boolean { + return true; + } + + /** + * Override to opt out when a per-session dependency is missing. Default: `true`. Gates the + * per-GLSP-session registry only — see {@link BaseMcpDiagramToolHandler.canRegister}. + */ canRegister(): boolean { return true; } diff --git a/packages/server-mcp/src/common/server/mcp-tool-handler.spec.ts b/packages/server-mcp/src/common/server/mcp-tool-handler.spec.ts index 466c7e3..2c57955 100644 --- a/packages/server-mcp/src/common/server/mcp-tool-handler.spec.ts +++ b/packages/server-mcp/src/common/server/mcp-tool-handler.spec.ts @@ -118,7 +118,7 @@ const matrix: Array<{ name: 'modify-nodes', Constructor: ModifyNodesMcpToolHandler, schema: ModifyNodesOutputSchema, - sample: { modifiedNodes: [{ id: 'n1', elementTypeId: 'node:foo' }], dispatchedCommands: 1, warnings: [] } + sample: { modifiedNodes: [{ id: 'n1', elementTypeId: 'node:foo' }], dispatchedCommands: 1, errors: [], warnings: [] } }, { name: 'modify-edges', diff --git a/packages/server-mcp/src/common/server/mcp-tool-handler.ts b/packages/server-mcp/src/common/server/mcp-tool-handler.ts index 851e208..c380388 100644 --- a/packages/server-mcp/src/common/server/mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/server/mcp-tool-handler.ts @@ -26,7 +26,7 @@ import { } from '@eclipse-glsp/server'; import { ToolAnnotations } from '@modelcontextprotocol/sdk/types'; import { inject, injectable, interfaces, optional } from 'inversify'; -import { ZodObject, ZodRawShape } from 'zod/v4'; +import { ZodObject, ZodRawShape, ZodType } from 'zod/v4'; import { GLSPMcpServer } from './glsp-mcp-server'; import { McpElementsNotFoundError, @@ -74,7 +74,7 @@ export const McpToolHandler = Symbol('McpToolHandler'); * @experimental */ @injectable() -export abstract class BaseMcpToolHandler { +export abstract class BaseMcpToolHandler { @inject(McpRequestContext) @optional() protected requestContext: McpRequestContext = new NoopMcpRequestContext(); @@ -95,9 +95,11 @@ export abstract class BaseMcpToolHandler { * Optional dual-emit schema. When set, pass the matching `structured` payload to * {@link success} so the framework forwards it as `structuredContent` alongside the * human-readable text. The MCP spec says clients SHOULD validate `structuredContent` - * against the declared schema, so the two MUST stay in sync. + * against the declared schema, so the two MUST stay in sync. Bind the `O` type parameter to + * `z.infer` to have the compiler enforce that — the type ties this + * field and {@link success}'s payload to the same shape. */ - readonly outputSchema?: ZodObject; + readonly outputSchema?: ZodObject & ZodType; /** Optional human-friendly display name for UIs that render a friendlier label than `name`. */ readonly title?: string; @@ -187,7 +189,7 @@ export abstract class BaseMcpToolHandler { * Pass `structured` whenever {@link outputSchema} is declared (the spec says clients SHOULD * validate against the declared shape). Omit for plain text-only responses. */ - protected success(message: string, structured?: McpStructuredContent): McpToolResult { + protected success(message: string, structured?: O): McpToolResult { return { isError: false, content: [{ type: 'text', text: message }], structuredContent: structured }; } @@ -220,13 +222,16 @@ export abstract class BaseMcpToolHandler { * @experimental */ @injectable() -export abstract class AbstractMcpToolHandler> extends BaseMcpToolHandler implements McpToolHandler { +export abstract class AbstractMcpToolHandler, O extends McpStructuredContent = McpStructuredContent> + extends BaseMcpToolHandler + implements McpToolHandler +{ /** Throw {@link McpToolError} for expected, user-facing errors; the base wraps. */ - protected abstract createResult(params: T): MaybePromise; + protected abstract createResult(params: I): MaybePromise; registerTool(server: GLSPMcpServer): void { server.registerTool(this.name, this.toRegistrationConfig(), async (params, extra) => - this.requestContext.run(extra, () => this.execute(() => this.createResult(params as T))) + this.requestContext.run(extra, () => this.execute(() => this.createResult(params as I))) ); } } @@ -247,14 +252,17 @@ export abstract class AbstractMcpToolHandler> extend * @experimental */ @injectable() -export abstract class BaseMcpDiagramToolHandler extends BaseMcpToolHandler { +export abstract class BaseMcpDiagramToolHandler< + I extends McpDiagramScopedInput = McpDiagramScopedInput, + O extends McpStructuredContent = McpStructuredContent +> extends BaseMcpToolHandler { @inject(ClientId) protected clientId: string; @inject(ModelState) protected modelState: ModelState; @inject(McpIdAliasService) protected aliasService: McpIdAliasService; @inject(McpLabelProvider) protected labelProvider: McpLabelProvider; /** Throw {@link McpToolError} for expected errors; the base wraps. */ - protected abstract createResult(params: T): MaybePromise; + protected abstract createResult(params: I): MaybePromise; /** * Public dispatch entry point invoked by {@link AbstractMcpServerLauncher}'s registered SDK @@ -262,9 +270,27 @@ export abstract class BaseMcpDiagramToolHandler; + abstract handle(params: I): Promise; - /** Override to opt out of registration when a runtime dependency is missing. Default: `true`. */ + /** + * Whether the diagram type can support this tool at all. Evaluated once per diagram type at + * MCP-server start against a container with that type's diagram modules loaded, so + * `@inject(...) @optional()` dependencies reflect its real bindings. When no registered + * diagram type supports the tool, it is left out of the MCP catalog entirely. + * + * Override this for statically bound dependencies (e.g. a `LayoutEngine`) and + * {@link canRegister} for capabilities of the connected GLSP client — the harvest container + * has no client, so client capabilities always read as absent here. + */ + isSupportedByDiagramType(): boolean { + return true; + } + + /** + * Override to opt out when a per-session dependency is missing. Default: `true`. Gates the + * per-GLSP-session registry only: the tool stays in the catalog (another session may support + * it) and calls against this session fail with "No tool handler '…' registered". + */ canRegister(): boolean { return true; } @@ -357,9 +383,10 @@ export abstract class BaseMcpDiagramToolHandler extends BaseMcpDiagramToolHandler { - handle(params: T): Promise { + I extends McpDiagramScopedInput = McpDiagramScopedInput, + O extends McpStructuredContent = McpStructuredContent +> extends BaseMcpDiagramToolHandler { + handle(params: I): Promise { return this.execute(() => this.createResult(params)); } } @@ -388,8 +415,9 @@ export abstract class AbstractMcpDiagramToolHandler< */ @injectable() export abstract class OperationMcpDiagramToolHandler< - T extends McpDiagramScopedInput = McpDiagramScopedInput -> extends BaseMcpDiagramToolHandler { + I extends McpDiagramScopedInput = McpDiagramScopedInput, + O extends McpStructuredContent = McpStructuredContent +> extends BaseMcpDiagramToolHandler { @inject(ActionDispatcher) protected actionDispatcher: ActionDispatcher; // Operation tools mutate the model — flip the read defaults. Concrete handlers override @@ -400,7 +428,7 @@ export abstract class OperationMcpDiagramToolHandler< override readonly destructiveHint: boolean = false; override readonly idempotentHint: boolean = false; - handle(params: T): Promise { + handle(params: I): Promise { return this.execute(() => { if (this.modelState.isReadonly) { throw new McpReadOnlyError(); @@ -431,5 +459,5 @@ export abstract class OperationMcpDiagramToolHandler< * are read off `new Constructor()` at MCP-session-init for SDK catalog registration, the same * trick `bindOperations` uses to read `operationType`. */ -export type McpDiagramToolHandlerConstructor = interfaces.Newable>; +export type McpDiagramToolHandlerConstructor = interfaces.Newable>; export const McpDiagramToolHandlerConstructor = Symbol('McpDiagramToolHandlerConstructor'); diff --git a/packages/server-mcp/src/common/tools/handlers/count-elements-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/count-elements-mcp-tool-handler.ts index 9191b58..c02b8e8 100644 --- a/packages/server-mcp/src/common/tools/handlers/count-elements-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/count-elements-mcp-tool-handler.ts @@ -27,6 +27,7 @@ export const CountElementsOutputSchema = z.object({ total: z.number().int().describe('Total element count across the diagram (root included).'), countsByType: z.record(z.string(), z.number().int()).describe('Element count grouped by `GModelElement.type`.') }); +export type CountElementsOutput = z.infer; /** * Counts elements in the diagram, grouped by type. Cheap alternative to dumping the full @@ -34,7 +35,7 @@ export const CountElementsOutputSchema = z.object({ * elements of type X exist". */ @injectable() -export class CountElementsMcpToolHandler extends AbstractMcpDiagramToolHandler { +export class CountElementsMcpToolHandler extends AbstractMcpDiagramToolHandler { static readonly NAME = 'count-elements'; readonly name = CountElementsMcpToolHandler.NAME; override readonly title = 'Count Diagram Elements'; diff --git a/packages/server-mcp/src/common/tools/handlers/create-edges-mcp-tool-handler.spec.ts b/packages/server-mcp/src/common/tools/handlers/create-edges-mcp-tool-handler.spec.ts index b881154..276c22b 100644 --- a/packages/server-mcp/src/common/tools/handlers/create-edges-mcp-tool-handler.spec.ts +++ b/packages/server-mcp/src/common/tools/handlers/create-edges-mcp-tool-handler.spec.ts @@ -32,6 +32,7 @@ import { McpToolResult } from '../../server/mcp-handler-shared'; import { McpIdAliasService } from '../../server/mcp-id-alias-service'; import { DefaultMcpLabelProvider, McpLabelProvider } from '../../server/mcp-label-provider'; import { CreateEdgesInput, CreateEdgesMcpToolHandler } from './create-edges-mcp-tool-handler'; +import { expectValidStructuredContent } from './test/expect-structured-content'; function makeElement(id: string, type: string): GModelElement { return { id, type, children: [] } as unknown as GModelElement; @@ -137,6 +138,7 @@ describe('CreateEdgesMcpToolHandler · dryRun', () => { baseInput({ edges: [{ elementTypeId: 'edge:static', sourceElementId: 's', targetElementId: 't' }] }) ); + expectValidStructuredContent(handler, result); const structured = result.structuredContent as unknown as DryRunStructured; expect(structured.validationResults).toHaveLength(1); expect(structured.validationResults[0].isValid).toBe(true); diff --git a/packages/server-mcp/src/common/tools/handlers/create-edges-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/create-edges-mcp-tool-handler.ts index 7e37cfe..f3df598 100644 --- a/packages/server-mcp/src/common/tools/handlers/create-edges-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/create-edges-mcp-tool-handler.ts @@ -14,7 +14,13 @@ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 ********************************************************************************/ -import { ChangeRoutingPointsOperation, CreateEdgeOperation, DiagramConfiguration, EdgeCreationChecker } from '@eclipse-glsp/server'; +import { + ChangeRoutingPointsOperation, + CreateEdgeOperation, + DiagramConfiguration, + EdgeCreationChecker, + GModelElement +} from '@eclipse-glsp/server'; import { inject, injectable, optional } from 'inversify'; import * as z from 'zod/v4'; import { McpToolResult } from '../../server/mcp-handler-shared'; @@ -66,12 +72,13 @@ export const CreateEdgesOutputSchema = z.object({ .optional() .describe('Per-input validation results. Present only in `dryRun` mode.') }); +export type CreateEdgesOutput = z.infer; type EdgeInput = CreateEdgesInput['edges'][number]; type ValidationResult = z.infer; @injectable() -export class CreateEdgesMcpToolHandler extends OperationMcpDiagramToolHandler { +export class CreateEdgesMcpToolHandler extends OperationMcpDiagramToolHandler { static readonly NAME = 'create-edges'; readonly name = CreateEdgesMcpToolHandler.NAME; override readonly title = 'Create Diagram Edges'; @@ -106,7 +113,7 @@ export class CreateEdgesMcpToolHandler extends OperationMcpDiagramToolHandler { @@ -137,11 +144,16 @@ export class CreateEdgesMcpToolHandler extends OperationMcpDiagramToolHandler !beforeIds.includes(id)); - const newElements = newIds.map(id => this.modelState.index.find(id)).filter(element => element?.type === elementTypeId); - const newElement = newElements[0]; - if (newElements.length > 1) { - this.logger.warn('More than 1 new element created'); + const knownIds = new Set(beforeIds); + const newElements = afterIds + .filter(id => !knownIds.has(id)) + .map(id => this.modelState.index.find(id)) + .filter((element): element is GModelElement => element !== undefined); + // Fall back to the first new element — see `create-nodes`. + const typeMatches = newElements.filter(element => element.type === elementTypeId); + const newElement = typeMatches[0] ?? newElements[0]; + if (newElements.length > 1 && typeMatches.length !== 1) { + this.logger.warn(`Ambiguous creation result for '${elementTypeId}': picked '${newElement?.id}' among new elements`); } beforeIds = afterIds; diff --git a/packages/server-mcp/src/common/tools/handlers/create-nodes-mcp-tool-handler.spec.ts b/packages/server-mcp/src/common/tools/handlers/create-nodes-mcp-tool-handler.spec.ts index fb53db1..7cee393 100644 --- a/packages/server-mcp/src/common/tools/handlers/create-nodes-mcp-tool-handler.spec.ts +++ b/packages/server-mcp/src/common/tools/handlers/create-nodes-mcp-tool-handler.spec.ts @@ -32,6 +32,7 @@ import { McpToolResult } from '../../server/mcp-handler-shared'; import { McpIdAliasService } from '../../server/mcp-id-alias-service'; import { McpLabelProvider } from '../../server/mcp-label-provider'; import { CreateNodesInput, CreateNodesMcpToolHandler } from './create-nodes-mcp-tool-handler'; +import { expectValidStructuredContent } from './test/expect-structured-content'; interface CreatedElementsStructured { createdNodes: { id: string; elementTypeId: string; label?: string }[]; @@ -145,6 +146,7 @@ describe('CreateNodesMcpToolHandler', () => { ] }); + expectValidStructuredContent(handler, result); const structured = result.structuredContent as unknown as CreatedElementsStructured; expect(structured.createdNodes).toHaveLength(2); expect(structured.createdNodes[0].id).toBe('task:manual#1'); diff --git a/packages/server-mcp/src/common/tools/handlers/create-nodes-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/create-nodes-mcp-tool-handler.ts index ca2e2bf..337d273 100644 --- a/packages/server-mcp/src/common/tools/handlers/create-nodes-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/create-nodes-mcp-tool-handler.ts @@ -14,7 +14,7 @@ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 ********************************************************************************/ -import { ApplyLabelEditOperation, CreateNodeOperation } from '@eclipse-glsp/server'; +import { ApplyLabelEditOperation, CreateNodeOperation, GModelElement } from '@eclipse-glsp/server'; import { injectable } from 'inversify'; import * as z from 'zod/v4'; import { McpToolResult } from '../../server/mcp-handler-shared'; @@ -33,7 +33,10 @@ export const CreateNodeSpecSchema = z.strictObject({ elementTypeId: z .string() .describe('Element type ID (e.g., `task:manual`, `task:automated`). Use the `element-types` tool to discover valid IDs.'), - position: position.describe('Position where the node should be created (absolute diagram coordinates)'), + position: position.describe( + 'Position where the node should be created, in absolute diagram coordinates — the server converts to ' + + 'parent-relative when `containerId` is set. Note `modify-nodes` takes parent-relative coordinates instead.' + ), text: z.string().optional().describe('Label text to use in case the given element type allows for labels.'), containerId: z.string().optional().describe('ID of the container element. If not provided, node is added to the root.'), // `args` stays open (`record(...)`) — adopter-specific extension surface for per-element-type creation hints. @@ -57,9 +60,10 @@ export const CreateNodesOutputSchema = z.object({ 'Soft notices for inputs that succeeded with caveats (e.g. `text` supplied for a type whose elements have no editable label).' ) }); +export type CreateNodesOutput = z.infer; @injectable() -export class CreateNodesMcpToolHandler extends OperationMcpDiagramToolHandler { +export class CreateNodesMcpToolHandler extends OperationMcpDiagramToolHandler { static readonly NAME = 'create-nodes'; readonly name = CreateNodesMcpToolHandler.NAME; override readonly title = 'Create Diagram Nodes'; @@ -91,11 +95,18 @@ export class CreateNodesMcpToolHandler extends OperationMcpDiagramToolHandler !beforeIds.includes(id)); - const newElements = newIds.map(id => this.modelState.index.find(id)).filter(element => element?.type === elementTypeId); - const newElement = newElements[0]; - if (newElements.length > 1) { - this.logger.warn('More than 1 new element created'); + const knownIds = new Set(beforeIds); + const newElements = afterIds + .filter(id => !knownIds.has(id)) + .map(id => this.modelState.index.find(id)) + .filter((element): element is GModelElement => element !== undefined); + // Fall back to the first new element: the built type may differ from the requested + // `elementTypeId` (type hint vs. concrete GModel type). `allIds()` is pre-order, so the + // first entry is the created element itself rather than one of its new children. + const typeMatches = newElements.filter(element => element.type === elementTypeId); + const newElement = typeMatches[0] ?? newElements[0]; + if (newElements.length > 1 && typeMatches.length !== 1) { + this.logger.warn(`Ambiguous creation result for '${elementTypeId}': picked '${newElement?.id}' among new elements`); } beforeIds = afterIds; diff --git a/packages/server-mcp/src/common/tools/handlers/delete-elements-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/delete-elements-mcp-tool-handler.ts index 4e86390..06ff764 100644 --- a/packages/server-mcp/src/common/tools/handlers/delete-elements-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/delete-elements-mcp-tool-handler.ts @@ -38,9 +38,10 @@ export const DeleteElementsOutputSchema = z.object({ ), dispatchedCommands }); +export type DeleteElementsOutput = z.infer; @injectable() -export class DeleteElementsMcpToolHandler extends OperationMcpDiagramToolHandler { +export class DeleteElementsMcpToolHandler extends OperationMcpDiagramToolHandler { static readonly NAME = 'delete-elements'; readonly name = DeleteElementsMcpToolHandler.NAME; override readonly title = 'Delete Diagram Elements'; diff --git a/packages/server-mcp/src/common/tools/handlers/diagram-model-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/diagram-model-mcp-tool-handler.ts index ff7d35b..b78185e 100644 --- a/packages/server-mcp/src/common/tools/handlers/diagram-model-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/diagram-model-mcp-tool-handler.ts @@ -38,9 +38,10 @@ export const DiagramModelOutputSchema = z.object({ sessionId: z.string(), elements: z.array(DiagramModelElementSchema) }); +export type DiagramModelOutput = z.infer; @injectable() -export class DiagramModelMcpToolHandler extends AbstractMcpDiagramToolHandler { +export class DiagramModelMcpToolHandler extends AbstractMcpDiagramToolHandler { static readonly NAME = 'diagram-model'; readonly name = DiagramModelMcpToolHandler.NAME; override readonly title = 'Diagram Model Structure'; @@ -57,8 +58,10 @@ export class DiagramModelMcpToolHandler extends AbstractMcpDiagramToolHandler; /** * Server-scope so the LLM can discover types before opening a session. Lazy-harvests the @@ -66,7 +67,7 @@ export const ElementTypesOutputSchema = z.object({ * `bindElementTypesProvider()`; the harvest picks it up without further overrides. */ @injectable() -export class ElementTypesMcpToolHandler extends AbstractMcpToolHandler { +export class ElementTypesMcpToolHandler extends AbstractMcpToolHandler { @inject(DiagramModules) protected diagramModules: Map; @inject(InjectionContainer) protected serverContainer: Container; @inject(ClientSessionManager) protected clientSessionManager: ClientSessionManager; @@ -89,8 +90,10 @@ export class ElementTypesMcpToolHandler extends AbstractMcpToolHandler; export const GetSelectionOutputSchema = z.object({ selectedElementIds: z.array(z.string()).describe('Aliased ids of the elements currently selected on the client.') }); +export type GetSelectionOutput = z.infer; /** Round-trips a sprotty {@link GetSelectionAction} via `ActionDispatcher.requestUntil` and awaits the matching `SelectionResult`. */ @injectable() -export class GetSelectionMcpToolHandler extends AbstractMcpDiagramToolHandler { +export class GetSelectionMcpToolHandler extends AbstractMcpDiagramToolHandler { /** Timeout (in ms) for awaiting the selection response from the client. Override via subclass + rebind. */ protected readonly timeoutMs: number = 5000; diff --git a/packages/server-mcp/src/common/tools/handlers/layout-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/layout-mcp-tool-handler.ts index 80e2485..947b4c8 100644 --- a/packages/server-mcp/src/common/tools/handlers/layout-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/layout-mcp-tool-handler.ts @@ -28,10 +28,11 @@ export const LayoutOutputSchema = z.object({ applied: z.boolean().describe('Always true on success — surfaced for parity with other operations.'), dispatchedCommands }); +export type LayoutOutput = z.infer; /** Not registered by default: requires an adopter-supplied `LayoutEngine` to bind, which only some GLSP servers ship. */ @injectable() -export class LayoutMcpToolHandler extends OperationMcpDiagramToolHandler { +export class LayoutMcpToolHandler extends OperationMcpDiagramToolHandler { static readonly NAME = 'layout'; readonly name = LayoutMcpToolHandler.NAME; override readonly title = 'Auto-Layout Diagram'; @@ -47,7 +48,12 @@ export class LayoutMcpToolHandler extends OperationMcpDiagramToolHandler; @injectable() -export class ModifyEdgesMcpToolHandler extends OperationMcpDiagramToolHandler { +export class ModifyEdgesMcpToolHandler extends OperationMcpDiagramToolHandler { static readonly NAME = 'modify-edges'; readonly name = ModifyEdgesMcpToolHandler.NAME; override readonly title = 'Modify Diagram Edges'; @@ -88,7 +89,6 @@ export class ModifyEdgesMcpToolHandler extends OperationMcpDiagramToolHandler; realId: string; inputId: string }> = []; const errors: string[] = []; - const modifiedRealIds = new Set(); elements.forEach(([change]) => { const { routingPoints } = change; const realId = this.aliasService.lookup(change.elementId); @@ -114,7 +114,6 @@ export class ModifyEdgesMcpToolHandler extends OperationMcpDiagramToolHandler entry.promise)); + const modifiedRealIds = new Set(); results.forEach((result, i) => { + const { realId, inputId } = dispatched[i]; if (result.status === 'rejected') { - const { realId, inputId } = dispatched[i]; const reason = result.reason instanceof Error ? result.reason.message : String(result.reason); errors.push(`Failed to modify edge '${inputId}': ${reason}`); - // The dispatch failed, so this id was *not* modified — drop it from the success list. - modifiedRealIds.delete(realId); + } else { + modifiedRealIds.add(realId); } }); @@ -141,7 +147,7 @@ export class ModifyEdgesMcpToolHandler extends OperationMcpDiagramToolHandler this.describeElement(realId)) .filter((entry): entry is NonNullable => entry !== undefined); return this.success( - `Successfully modified ${edges.length - errors.length} edge(s) (in ${dispatched.length} commands)${formatNoticeList('errors', errors)}`, + `Successfully modified ${modifiedEdges.length} edge(s) (in ${dispatched.length} commands)${formatNoticeList('errors', errors)}`, { modifiedEdges, dispatchedCommands: dispatched.length, errors } ); } diff --git a/packages/server-mcp/src/common/tools/handlers/modify-nodes-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/modify-nodes-mcp-tool-handler.ts index 85f4864..6327fbe 100644 --- a/packages/server-mcp/src/common/tools/handlers/modify-nodes-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/modify-nodes-mcp-tool-handler.ts @@ -14,7 +14,7 @@ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 ********************************************************************************/ -import { ApplyLabelEditOperation, ChangeBoundsOperation, GEdge, GShapeElement } from '@eclipse-glsp/server'; +import { ApplyLabelEditOperation, ChangeBoundsOperation, GEdge, GModelRoot, GNode } from '@eclipse-glsp/server'; import { injectable } from 'inversify'; import * as z from 'zod/v4'; import { McpToolError, McpToolResult } from '../../server/mcp-handler-shared'; @@ -37,7 +37,13 @@ export const NodeSizeSchema = z.strictObject({ /** Single node-modification entry. Strict so an LLM-typoed field surfaces as a validation error instead of being silently dropped. */ export const ModifyNodeSpecSchema = z.strictObject({ elementId, - position: position.optional().describe('Position where the node should be moved to (absolute diagram coordinates)'), + position: position + .optional() + .describe( + 'Position where the node should be moved to, relative to the parent element ' + + '(identical to absolute diagram coordinates for direct children of the root, which is the common case). ' + + 'Matches the `position` reported by `query-elements`. Note `create-nodes` takes absolute coordinates instead.' + ), size: NodeSizeSchema.optional().describe('New size of the node'), text: z.string().optional().describe("Label text to use instead (given that the element's type allows for labels).") }); @@ -55,15 +61,17 @@ export const ModifyNodesOutputSchema = z.object({ .array(ElementIdentitySchema) .describe('Identity of each node whose change request was dispatched (post-modification labels).'), dispatchedCommands, + errors: z.array(z.string()).describe('Per-input failure messages; absent or empty when every input succeeded.'), warnings: z .array(z.string()) .describe( 'Soft notices for inputs whose change applied with caveats (e.g. `text` supplied for a node whose type has no editable label).' ) }); +export type ModifyNodesOutput = z.infer; @injectable() -export class ModifyNodesMcpToolHandler extends OperationMcpDiagramToolHandler { +export class ModifyNodesMcpToolHandler extends OperationMcpDiagramToolHandler { static readonly NAME = 'modify-nodes'; readonly name = ModifyNodesMcpToolHandler.NAME; override readonly title = 'Modify Diagram Nodes'; @@ -88,53 +96,83 @@ export class ModifyNodesMcpToolHandler extends OperationMcpDiagramToolHandler !(element instanceof GShapeElement)).map(([change]) => `'${change.elementId}'`); - if (nonShape.length) { + // The root has no bounds of its own, and a `text` edit against it would rename whatever + // top-level `GLabel` the label provider happens to find first. + const roots = elements.filter(([, element]) => element instanceof GModelRoot).map(([change]) => `'${change.elementId}'`); + if (roots.length) { + throw new McpToolError( + `modify-nodes does not accept the diagram root — got: ${roots.join(', ')}. Target a node inside the diagram instead.` + ); + } + + // Core's `GModelChangeBoundsOperationHandler` only applies bounds to a `GNode` + // (`findByClass`). Label-only edits stay open to every element kind. + const unmovable = elements + .filter(([change, element]) => (change.position || change.size) && !(element instanceof GNode)) + .map(([change, element]) => `'${change.elementId}' (type '${element.type}')`); + if (unmovable.length) { throw new McpToolError( - `modify-nodes only accepts shape elements — got: ${nonShape.join(', ')}. ` + - 'Use `query-elements` (inspect mode) to find shape ids, or pick the parent shape.' + `modify-nodes can only change \`position\` / \`size\` of nodes — got: ${unmovable.join(', ')}. ` + + 'Use `query-elements` (inspect mode) to find node ids, or target the parent node.' ); } - // Modifications are independent of each other — dispatch in parallel. - const promises: Promise[] = []; + // Modifications are independent of each other — dispatch in parallel. `allSettled` so one + // failed dispatch surfaces in `errors` instead of rejecting the whole call and losing the + // other outcomes, which have already mutated the model. + const dispatched: Array<{ promise: Promise; inputId: string }> = []; + const errors: string[] = []; const warnings: string[] = []; elements.forEach(([change, element]) => { - // Guaranteed non-null and shape-typed by the missing-elements / nonShape checks above. - const resolved = element as GShapeElement; const { size, position, text } = change; const realId = this.aliasService.lookup(change.elementId); - if (size || position) { - const newSize = size ?? resolved.size; - const newPosition = position ?? resolved.position; + if (!size && !position && !text) { + errors.push(`No change requested for node '${change.elementId}' — provide \`position\`, \`size\` or a non-empty \`text\`.`); + return; + } + + if ((size || position) && element instanceof GNode) { + const newSize = size ?? element.size; + const newPosition = position ?? element.position; const operation = ChangeBoundsOperation.create([{ elementId: realId, newSize, newPosition }]); - promises.push(this.actionDispatcher.dispatch(operation)); + dispatched.push({ promise: this.actionDispatcher.dispatch(operation), inputId: change.elementId }); } if (text) { - const labelId = this.labelProvider.getLabel(resolved)?.id; + const labelId = this.labelProvider.getLabel(element)?.id; if (labelId) { - promises.push(this.actionDispatcher.dispatch(ApplyLabelEditOperation.create({ labelId, text }))); + const operation = ApplyLabelEditOperation.create({ labelId, text }); + dispatched.push({ promise: this.actionDispatcher.dispatch(operation), inputId: change.elementId }); } else { warnings.push( - `Ignored \`text\` for '${change.elementId}' (type '${resolved.type}') — this element has no editable label.` + `Ignored \`text\` for '${change.elementId}' (type '${element.type}') — this element has no editable label.` ); } } }); - await Promise.all(promises); + const results = await Promise.allSettled(dispatched.map(entry => entry.promise)); + const modifiedInputIds = new Set(); + results.forEach((result, index) => { + const { inputId } = dispatched[index]; + if (result.status === 'rejected') { + const reason = result.reason instanceof Error ? result.reason.message : String(result.reason); + errors.push(`Failed to modify node '${inputId}': ${reason}`); + } else { + modifiedInputIds.add(inputId); + } + }); - const modifiedNodes = nodes - .map(change => this.describeElement(change.elementId)) + const modifiedNodes = [...modifiedInputIds] + .map(inputId => this.describeElement(inputId)) .filter((entry): entry is NonNullable => entry !== undefined); return this.success( - `Successfully modified ${nodes.length} node(s) (in ${promises.length} commands)` + formatNoticeList('warnings', warnings), - { modifiedNodes, dispatchedCommands: promises.length, warnings } + `Successfully modified ${modifiedNodes.length} node(s) (in ${dispatched.length} commands)` + + formatNoticeList('errors', errors) + + formatNoticeList('warnings', warnings), + { modifiedNodes, dispatchedCommands: dispatched.length, errors, warnings } ); } } diff --git a/packages/server-mcp/src/common/tools/handlers/query-elements-mcp-tool-handler.spec.ts b/packages/server-mcp/src/common/tools/handlers/query-elements-mcp-tool-handler.spec.ts index 26dd635..157350f 100644 --- a/packages/server-mcp/src/common/tools/handlers/query-elements-mcp-tool-handler.spec.ts +++ b/packages/server-mcp/src/common/tools/handlers/query-elements-mcp-tool-handler.spec.ts @@ -22,6 +22,7 @@ import { McpElementsNotFoundError, McpToolResult } from '../../server/mcp-handle import { McpIdAliasService } from '../../server/mcp-id-alias-service'; import { DefaultMcpLabelProvider, McpLabelProvider } from '../../server/mcp-label-provider'; import { QueryElementsInput, QueryElementsMcpToolHandler } from './query-elements-mcp-tool-handler'; +import { expectValidStructuredContent } from './test/expect-structured-content'; function makeLabel(text: string): GLabel { // Set the prototype so `child instanceof GLabel` checks in the handler return true. @@ -109,6 +110,7 @@ describe('QueryElementsMcpToolHandler', () => { ]); const result = await callCreateResult(handler, { sessionId: 's', types: ['task:manual'] }); + expectValidStructuredContent(handler, result); const structured = result.structuredContent as unknown as ListStructured; expect(structured.mode).toBe('list'); expect(structured.matches).toHaveLength(1); @@ -179,6 +181,7 @@ describe('QueryElementsMcpToolHandler', () => { const finalCall = serializer.capturedArrays[serializer.capturedArrays.length - 1]; expect(finalCall).toEqual(elements); + expectValidStructuredContent(handler, result); const structured = result.structuredContent as unknown as InspectStructured; expect(structured.mode).toBe('inspect'); }); diff --git a/packages/server-mcp/src/common/tools/handlers/query-elements-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/query-elements-mcp-tool-handler.ts index 7958804..95df4cb 100644 --- a/packages/server-mcp/src/common/tools/handlers/query-elements-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/query-elements-mcp-tool-handler.ts @@ -60,10 +60,11 @@ export const QueryElementsOutputSchema = z.object({ 'The `elements` array then includes the container plus its descendants.' ) }); +export type QueryElementsOutput = z.infer; /** Two-mode element query — list/filter or inspect-by-id, discriminated by `elementIds` presence. */ @injectable() -export class QueryElementsMcpToolHandler extends AbstractMcpDiagramToolHandler { +export class QueryElementsMcpToolHandler extends AbstractMcpDiagramToolHandler { static readonly NAME = 'query-elements'; readonly name = QueryElementsMcpToolHandler.NAME; override readonly title = 'Query Diagram Elements'; diff --git a/packages/server-mcp/src/common/tools/handlers/redo-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/redo-mcp-tool-handler.ts index 727c617..9f56a72 100644 --- a/packages/server-mcp/src/common/tools/handlers/redo-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/redo-mcp-tool-handler.ts @@ -22,19 +22,20 @@ import { McpDiagramScopedInputSchema } from '../../server/mcp-input-schemas'; import { OperationMcpDiagramToolHandler } from '../../server/mcp-tool-handler'; export const RedoInputSchema = McpDiagramScopedInputSchema.extend({ - commandsToRedo: z.number().min(1).default(1).describe('Number of commands to redo. Defaults to 1 (most recent undone command).') + commandsToRedo: z.number().int().min(1).default(1).describe('Number of commands to redo. Defaults to 1 (most recent undone command).') }); export type RedoInput = z.infer; export const RedoOutputSchema = z.object({ commandsRedone: z.number().int().describe('Number of previously-undone commands re-applied.') }); +export type RedoOutput = z.infer; /** * Redo a given number of the most recent undone actions on the command stack. */ @injectable() -export class RedoMcpToolHandler extends OperationMcpDiagramToolHandler { +export class RedoMcpToolHandler extends OperationMcpDiagramToolHandler { static readonly NAME = 'redo'; readonly name = RedoMcpToolHandler.NAME; override readonly title = 'Redo Diagram Commands'; @@ -55,10 +56,14 @@ export class RedoMcpToolHandler extends OperationMcpDiagramToolHandler; /** Doesn't extend `OperationMcpDiagramToolHandler` — saving a read-only model is a legitimate adopter scenario (e.g. "save as" to a writable location). */ @injectable() -export class SaveModelMcpToolHandler extends AbstractMcpDiagramToolHandler { +export class SaveModelMcpToolHandler extends AbstractMcpDiagramToolHandler { static readonly NAME = 'save-model'; readonly name = SaveModelMcpToolHandler.NAME; override readonly title = 'Save Diagram Model'; @@ -62,7 +63,9 @@ export class SaveModelMcpToolHandler extends AbstractMcpDiagramToolHandler { - if (!this.commandStack.isDirty) { + // Only save-in-place can be short-circuited: a "save as" destination has no copy yet, + // dirty or not. + if (!fileUri && !this.commandStack.isDirty) { this.mcpLogger.info('save-model: nothing to save'); return this.success('No changes to save', { saved: false, fileUri }); } diff --git a/packages/server-mcp/src/common/tools/handlers/session-info-mcp-tool-handler.spec.ts b/packages/server-mcp/src/common/tools/handlers/session-info-mcp-tool-handler.spec.ts index 82f5712..d0cc536 100644 --- a/packages/server-mcp/src/common/tools/handlers/session-info-mcp-tool-handler.spec.ts +++ b/packages/server-mcp/src/common/tools/handlers/session-info-mcp-tool-handler.spec.ts @@ -19,6 +19,7 @@ import { describe, expect, it } from 'vitest'; import { Container, ContainerModule } from 'inversify'; import { McpToolResult } from '../../server/mcp-handler-shared'; import { SessionInfoMcpToolHandler } from './session-info-mcp-tool-handler'; +import { expectValidStructuredContent } from './test/expect-structured-content'; function asText(result: McpToolResult): string { const block = result.content[0]; @@ -115,6 +116,7 @@ describe('SessionInfoMcpToolHandler', () => { expect(text).toContain('workflow-diagram'); expect(text).toContain('state-diagram'); // Full per-session detail goes via structuredContent. + expectValidStructuredContent(handler, result); const sessions = (result.structuredContent as { sessions: SessionRow[] }).sessions; expect(sessions.map(row => row.sessionId)).toEqual(['session-alpha', 'session-beta']); }); diff --git a/packages/server-mcp/src/common/tools/handlers/session-info-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/session-info-mcp-tool-handler.ts index 6fd9b6a..ba972a7 100644 --- a/packages/server-mcp/src/common/tools/handlers/session-info-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/session-info-mcp-tool-handler.ts @@ -40,9 +40,10 @@ export type SessionInfoRow = z.infer; export const SessionInfoOutputSchema = z.object({ sessions: z.array(SessionInfoRowSchema) }); +export type SessionInfoOutput = z.infer; @injectable() -export class SessionInfoMcpToolHandler extends AbstractMcpToolHandler { +export class SessionInfoMcpToolHandler extends AbstractMcpToolHandler { @inject(ClientSessionManager) protected clientSessionManager: ClientSessionManager; static readonly NAME = 'session-info'; diff --git a/packages/server-mcp/src/common/tools/handlers/set-selection-mcp-tool-handler.spec.ts b/packages/server-mcp/src/common/tools/handlers/set-selection-mcp-tool-handler.spec.ts index 3b9e036..f573960 100644 --- a/packages/server-mcp/src/common/tools/handlers/set-selection-mcp-tool-handler.spec.ts +++ b/packages/server-mcp/src/common/tools/handlers/set-selection-mcp-tool-handler.spec.ts @@ -30,7 +30,8 @@ import { Container, ContainerModule } from 'inversify'; import { McpElementsNotFoundError, McpToolResult } from '../../server/mcp-handler-shared'; import { McpIdAliasService } from '../../server/mcp-id-alias-service'; import { DefaultMcpLabelProvider, McpLabelProvider } from '../../server/mcp-label-provider'; -import { SetSelectionInput, SetSelectionMcpToolHandler } from './set-selection-mcp-tool-handler'; +import { SetSelectionInput, SetSelectionInputSchema, SetSelectionMcpToolHandler } from './set-selection-mcp-tool-handler'; +import { expectValidStructuredContent } from './test/expect-structured-content'; function makeElement(id: string, type: string): GModelElement { return { id, type, children: [] } as unknown as GModelElement; @@ -97,14 +98,36 @@ describe('SetSelectionMcpToolHandler', () => { const { dispatcher, dispatched } = makeRecordingDispatcher(); const handler = buildHandler([makeElement('a', 'task'), makeElement('b', 'task'), makeElement('c', 'task')], dispatcher); - await callCreateResult(handler, { sessionId: 's', selectedElementIds: ['a', 'b'], deselectedElementIds: ['c'] }); + const result = await callCreateResult(handler, { + sessionId: 's', + selectedElementIds: ['a', 'b'], + deselectedElementIds: ['c'] + }); + expectValidStructuredContent(handler, result); expect(dispatched).toHaveLength(1); const action = dispatched[0] as SelectAction; expect(action.selectedElementsIDs).toEqual(['a', 'b']); expect(action.deselectedElementsIDs).toEqual(['c']); }); + it('clears the selection for the documented empty-array form, without a follow-up SelectAction', async () => { + const { dispatcher, dispatched } = makeRecordingDispatcher(); + const handler = buildHandler([makeElement('n1', 'task')], dispatcher); + const input = { sessionId: 's', selectedElementIds: [], clear: true }; + + // The empty array has to survive input validation before the handler ever sees it. + const parsed = SetSelectionInputSchema.safeParse(input); + expect(parsed.success ? [] : parsed.error.issues).toEqual([]); + + const result = await callCreateResult(handler, input); + + expectValidStructuredContent(handler, result); + expect(dispatched).toHaveLength(1); + expect(SelectAllAction.is(dispatched[0])).toBe(true); + expect((dispatched[0] as SelectAllAction).select).toBe(false); + }); + it('throws McpElementsNotFoundError when an id is missing from the model', async () => { const { dispatcher } = makeRecordingDispatcher(); const handler = buildHandler([makeElement('n1', 'task')], dispatcher); diff --git a/packages/server-mcp/src/common/tools/handlers/set-selection-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/set-selection-mcp-tool-handler.ts index f91fdb9..4ac8ef6 100644 --- a/packages/server-mcp/src/common/tools/handlers/set-selection-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/set-selection-mcp-tool-handler.ts @@ -18,14 +18,16 @@ import { ActionDispatcher, SelectAction, SelectAllAction } from '@eclipse-glsp/s import { inject, injectable } from 'inversify'; import * as z from 'zod/v4'; import { McpToolResult } from '../../server/mcp-handler-shared'; -import { McpDiagramScopedInputSchema, elementIds } from '../../server/mcp-input-schemas'; +import { McpDiagramScopedInputSchema, elementIdsAllowingEmpty } from '../../server/mcp-input-schemas'; import { AbstractMcpDiagramToolHandler } from '../../server/mcp-tool-handler'; export const SetSelectionInputSchema = McpDiagramScopedInputSchema.extend({ - selectedElementIds: elementIds + selectedElementIds: elementIdsAllowingEmpty .optional() - .describe('Element IDs to select. Pass an empty array (or omit + set `clear: true`) to clear the selection.'), - deselectedElementIds: elementIds.optional().describe('Element IDs to remove from the selection. Used to subtract without replacing.'), + .describe('Element IDs to select. Pass an empty array (or omit the field) together with `clear: true` to clear the selection.'), + deselectedElementIds: elementIdsAllowingEmpty + .optional() + .describe('Element IDs to remove from the selection. Used to subtract without replacing.'), clear: z .boolean() .optional() @@ -38,6 +40,7 @@ export const SetSelectionOutputSchema = z.object({ deselectedElementIds: z.array(z.string()).describe('Aliased ids requested for deselection.'), cleared: z.boolean().describe('Whether the existing selection was cleared before applying.') }); +export type SetSelectionOutput = z.infer; /** * Pushes a selection change to the client. Counterpart to `get-selection` — useful for the @@ -48,7 +51,7 @@ export const SetSelectionOutputSchema = z.object({ * even though the underlying GModel is unaffected. */ @injectable() -export class SetSelectionMcpToolHandler extends AbstractMcpDiagramToolHandler { +export class SetSelectionMcpToolHandler extends AbstractMcpDiagramToolHandler { static readonly NAME = 'set-selection'; readonly name = SetSelectionMcpToolHandler.NAME; override readonly title = 'Set Diagram Selection'; diff --git a/packages/server-mcp/src/common/tools/handlers/set-view-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/set-view-mcp-tool-handler.ts index e12a867..6b3b380 100644 --- a/packages/server-mcp/src/common/tools/handlers/set-view-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/set-view-mcp-tool-handler.ts @@ -74,9 +74,10 @@ export const SetViewOutputSchema = z.object({ .optional() .describe("Resolved final viewport when `action: 'set-viewport'` was applied (current values merged with the supplied overrides).") }); +export type SetViewOutput = z.infer; @injectable() -export class SetViewMcpToolHandler extends AbstractMcpDiagramToolHandler { +export class SetViewMcpToolHandler extends AbstractMcpDiagramToolHandler { /** Timeout (in ms) for the `GetViewportAction` round-trip used by `set-viewport` partial updates. Override via subclass + rebind. */ protected readonly viewportQueryTimeoutMs: number = 5000; @@ -102,7 +103,7 @@ export class SetViewMcpToolHandler extends AbstractMcpDiagramToolHandler this.aliasService.lookup(id)) : this.modelState.index.allIds(); + const resolvedIds = elementIds ? this.resolveExistingIds(elementIds) : this.modelState.index.allIds(); const dispatchAction = this.buildIntentAction(action, resolvedIds); await this.actionDispatcher.dispatch(dispatchAction); return this.success('Viewport successfully changed', { action, targetIds: this.encodeIds(resolvedIds) }); diff --git a/packages/server-mcp/src/common/tools/handlers/test/expect-structured-content.ts b/packages/server-mcp/src/common/tools/handlers/test/expect-structured-content.ts new file mode 100644 index 0000000..4c14727 --- /dev/null +++ b/packages/server-mcp/src/common/tools/handlers/test/expect-structured-content.ts @@ -0,0 +1,34 @@ +/******************************************************************************** + * Copyright (c) 2026 EclipseSource and others. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the Eclipse + * Public License v. 2.0 are satisfied: GNU General Public License, version 2 + * with the GNU Classpath Exception which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + ********************************************************************************/ + +import { expect } from 'vitest'; +import { McpToolResult } from '../../../server/mcp-handler-shared'; +import { BaseMcpToolHandler } from '../../../server/mcp-tool-handler'; + +/** + * Asserts that a tool result satisfies the handler's declared `outputSchema`. The MCP SDK runs + * this check on every `tools/call` and replaces the result with an error result when it fails, + * which a spec calling `createResult` directly does not exercise. + * + * Takes the handler rather than a schema so the assertion is always made against the schema the + * handler actually declares. + */ +export function expectValidStructuredContent(handler: Pick, result: McpToolResult): void { + const schema = handler.outputSchema; + expect(schema, 'handler declares no outputSchema, so there is nothing to validate against').toBeDefined(); + const parsed = schema!.safeParse(result.structuredContent); + expect(parsed.success ? [] : parsed.error.issues).toEqual([]); +} diff --git a/packages/server-mcp/src/common/tools/handlers/undo-mcp-tool-handler.ts b/packages/server-mcp/src/common/tools/handlers/undo-mcp-tool-handler.ts index 89f4c80..26d0850 100644 --- a/packages/server-mcp/src/common/tools/handlers/undo-mcp-tool-handler.ts +++ b/packages/server-mcp/src/common/tools/handlers/undo-mcp-tool-handler.ts @@ -22,19 +22,20 @@ import { McpDiagramScopedInputSchema } from '../../server/mcp-input-schemas'; import { OperationMcpDiagramToolHandler } from '../../server/mcp-tool-handler'; export const UndoInputSchema = McpDiagramScopedInputSchema.extend({ - commandsToUndo: z.number().min(1).default(1).describe('Number of commands to undo. Defaults to 1 (most recent command).') + commandsToUndo: z.number().int().min(1).default(1).describe('Number of commands to undo. Defaults to 1 (most recent command).') }); export type UndoInput = z.infer; export const UndoOutputSchema = z.object({ commandsUndone: z.number().int().describe('Number of commands actually reverted.') }); +export type UndoOutput = z.infer; /** * Undo a given number of the most recent actions on the command stack. */ @injectable() -export class UndoMcpToolHandler extends OperationMcpDiagramToolHandler { +export class UndoMcpToolHandler extends OperationMcpDiagramToolHandler { static readonly NAME = 'undo'; readonly name = UndoMcpToolHandler.NAME; override readonly title = 'Undo Diagram Commands'; @@ -54,10 +55,14 @@ export class UndoMcpToolHandler extends OperationMcpDiagramToolHandler7l30@Jl9`xZl$Z-t zvUxLS1)EYyMrN^IQEG91PFbpJMP_kHW?s5yO0h<2PHJvyUI~!7IYz*RRXV2Bz`!s~ fr8+mUC_A-CFDEf6HK!ILIC-O}C>xY{TyzNlNoz3f delta 122 zcmcbt`&ws%6X)c~oF$v(xJubfQgc#sQ}aqZQ;PL+6ALslQxt6N6iPBOi}ez7G82n~ zQ;W(nlT-C_^7FGxLDHI<3RaUp3W#q$FQCP$&Y)7An^=^cTBMhgn3S4Ri{PiE7AF^F Q7L;V>=hZSy))!j>0F