Correct MCP tool result contracts and registration - #152
Conversation
cad9806 to
a73f69e
Compare
| const structured = this.serializer.serializeStructured(root); | ||
| const count = Array.isArray(structured.elements) ? structured.elements.length : 0; | ||
| return this.success(this.summarizeModel(root, count), { sessionId, ...structured }); | ||
| const elements = (Array.isArray(structured.elements) ? structured.elements : []) as DiagramModelOutput['elements']; |
There was a problem hiding this comment.
This isn't type-neutral: the SDK only validates structuredContent, it doesn't strip or re-emit the parsed object, so extra top-level keys an adopter's serializeStructured returns used to reach the client and now get dropped. query-elements keeps the spread for the same serializer contract (link), so the two sibling handlers now disagree. Either spread here too, or drop the spread there.
| defaultHook: DiagramTypeSupportAware['isSupportedByDiagramType'] | ||
| ): C[] { | ||
| return constructors.filter(constructor => { | ||
| if (constructor.prototype.isSupportedByDiagramType === defaultHook) { |
There was a problem hiding this comment.
A constructor whose prototype has no isSupportedByDiagramType at all (duck-typed handler, not extending the base) fails this identity check, gets resolved through DI, throws TypeError: ... is not a function, and lands in the fail-open catch with a misleading "Could not probe" line. A typeof constructor.prototype.isSupportedByDiagramType !== 'function' short-circuit next to the identity check keeps it out of the probe entirely.
| if (nonShape.length) { | ||
| // Core's `GModelChangeBoundsOperationHandler` only applies bounds to a `GNode` | ||
| // (`findByClass`). Label-only edits stay open to every element kind. | ||
| const unmovable = elements |
There was a problem hiding this comment.
Narrowing the guard to bounds-only also widens what the tool accepts for label edits: GGraph/GModelRoot is not a GShapeElement, so it used to be rejected by the old blanket check (link) and now passes for a text-only entry. On a diagram with a free-floating top-level GLabel, { elementId: '<root>', text: 'x' } renames that label and reports the root as modified.
| * 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. | ||
| */ | ||
| export function expectValidStructuredContent(schema: ZodObject<ZodRawShape>, result: McpToolResult): void { |
There was a problem hiding this comment.
Taking the schema as a parameter means a spec can pass the wrong schema and still go green, which is the failure mode this helper exists to prevent. Take the handler and read handler.outputSchema instead; it's public and the matrix test already relies on that link (link).
| * against the declared schema, so the two MUST stay in sync. Bind the `O` type parameter to | ||
| * `z.infer<typeof MyOutputSchema>` to have the compiler enforce that. | ||
| */ | ||
| readonly outputSchema?: ZodObject<ZodRawShape>; |
There was a problem hiding this comment.
The new O parameter isn't tied to this field, so a subclass can declare O = FooOutput while assigning outputSchema = BarOutputSchema and still compile. Typing it as something like ZodObject<ZodRawShape> & ZodType<O> would actually close the loop the PR description claims (.shape access in toRegistrationConfig still works with the intersection).
|
|
||
| export const SetSelectionInputSchema = McpDiagramScopedInputSchema.extend({ | ||
| selectedElementIds: elementIds | ||
| selectedElementIds: z |
There was a problem hiding this comment.
This diverges from the shared fragment convention documented right above elementIds (link) without saying why, and the reason isn't obvious (Zod can't relax an existing .min(1)). Either add a shared empty-allowing fragment next to elementIds, or leave a one-liner here pointing at the constraint.
| expect(action.deselectedElementsIDs).toEqual(['c']); | ||
| }); | ||
|
|
||
| it('accepts the documented empty-array form for clearing the selection', () => { |
There was a problem hiding this comment.
The test name says "for clearing the selection" but this only parses the schema, it never runs the handler or asserts that SelectAllAction.create(false) gets dispatched. Either rename it to reflect that it's a schema test, or drive it through callCreateResult like the case above.
| - 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] Write the `validate-diagram` dedup separator as a `\u001f` escape, so the source file is no longer classified as binary by git [#152](https://github.com/eclipse-glsp/glsp-server-node/pull/152) | ||
| - [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 |
There was a problem hiding this comment.
This one is source-file hygiene rather than user-facing behaviour, adopters see no difference. I'd drop it from the changelog and keep it in the commit message.
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
a73f69e to
7773dd6
Compare
|
Thanks Tobias, all eight are addressed and pushed. Two of them were more right than I realised. The duck typed handler probe and the outputSchema gap were both already visible in this PR's own tests, I just hadn't noticed. The schema binding is now closed in both directions, I checked by deliberately declaring the wrong schema and confirming it no longer compiles. The NUL bytes turned out to predate this PR, they came in with 2.7.0, but I fixed them here anyway since the file was open. Dropped that line from the changelog as you suggested. One thing I did differently: for set-selection I added a shared elementIdsAllowingEmpty fragment next to elementIds instead of a comment at the call site, so there is no divergence left to explain. |
tortmayr
left a comment
tortmayr
left a comment
There was a problem hiding this comment.
Thanks for addressing all the issues.
LGMT! 👍🏼
2 participants
A review of the MCP tool handlers in
@eclipse-glsp/server-mcpsurfaced a set ofresult-contract and input-schema defects. This fixes them and closes the class of
bug at the type level.
Result contracts
The SDK validates
CallToolResult.structuredContentagainst the declaredoutputSchemaand, on a mismatch, replaces the entire result with an errorresult.
create-edgeswithdryRun: trueomitted the requireddispatchedCommands, so the dry run never returned its verdicts — the LLM sawOutput validation error: Invalid structured content for tool create-edges.To stop that recurring, the tool handler bases take an optional output type
parameter
O, and bothoutputSchemaand thesuccess()payload are typedagainst it. That closes the loop in both directions: emitting a payload that
does not satisfy the schema fails to compile (removing the fix now fails
tscwith
Property 'dispatchedCommands' is missing), and declaring a schema whoseshape differs from
Ofails too (Property 'commandsRedone' is missing).Behaviour
save-modelcheckedisDirtybefore consideringfileUri, so a "save as" toa new location on a clean model reported success and wrote nothing.
modify-nodesaccepted anyGShapeElement, but core'sGModelChangeBoundsOperationHandlerresolves viafindByClass(elementId, GNode)and returns early for anything else — a compartment or port silently no-opped
while the tool reported success.
undo/redodispatched N times without re-checking the stack and reportedthe requested count as the count actually applied. Their inputs also allowed
non-integers, which then failed the
.int()output schema.validate-diagramandset-viewalias-resolved ids without an existencecheck;
index.getAlldrops unknown ids silently, so a hallucinated id cameback as an empty marker list that reads as "diagram is clean".
modify-edgescounted entries that requested no change as modified, and afailure on one entry dropped a second entry's success for the same edge.
set-selectiondocumented an empty array as the way to clear a selection,which the shared
.min(1)schema fragment rejected.create-*tools inferred the new element by filtering the id diff ontype === elementTypeId, reporting a false "creation likely failed" foradopters whose handler builds a different concrete type.
Registration
canRegister()was answering two questions at once: whether a dependency isbound for the diagram type, and whether the connected GLSP client supports an
action. Only the first is knowable when the MCP catalog is built, and the
catalog consulted neither — so
layoutwas advertised on servers with noLayoutEngineand failed at call time.The hooks are now split.
isSupportedByDiagramType()is evaluated once perdiagram type during harvest, against a container with that type's modules
loaded, and gates catalog registration.
canRegister()is unchanged and stillgates the per-session registry, since the harvest container has no client and
would read every client capability as absent — which is why the export
resources must keep using it.
Verified against the real workflow modules: all 16 handlers resolve at harvest,
and
layoutis registered withElkLayoutModuleand dropped without it.Coverage
Handler specs called
createResultdirectly and cast the result, so nothingexercised the declared output schema — which is why the
create-edgesdefectsurvived. Specs now round-trip
structuredContentthrough the handler's ownoutputSchema, plus tests for the empty-array selection form and theregistration gate.