Skip to content

Comments

Mcp tool schema update#26

Merged
tomasfejfar merged 6 commits intokeboola:mainfrom
mariankrotil:mcp-mcp-tool-schema-update
Sep 4, 2025
Merged

Mcp tool schema update#26
tomasfejfar merged 6 commits intokeboola:mainfrom
mariankrotil:mcp-mcp-tool-schema-update

Conversation

@mariankrotil
Copy link
Contributor

@mariankrotil mariankrotil commented Sep 3, 2025

Before asking for review make sure that:

  • you created a task for KBC team to update event-schema in Connection (validation in API endpoint)
  • you notified everyone in #general that even schema is changing

@mariankrotil
Copy link
Contributor Author

KBC team task https://keboola.atlassian.net/browse/CT-2220

@mariankrotil
Copy link
Contributor Author

@Matovidlo Could you please notify in #general that mcp server tool event schema will be changed. And please review this PR and check my steps whether they are going the right direction.

@tomasfejfar
Copy link
Contributor

tomasfejfar commented Sep 4, 2025

Just one note - won't it fail for previous versions of MCP server which sends events without the requires param? The validation is strict in connection - either it matches the schema or it's refused.

@mariankrotil
Copy link
Contributor Author

Just one note - won't it fail for previous versions of MCP server which sends events without the requires param? The validation is strict in connection - either it matches the schema or it's refused.

I was thinking the same. @ujovlado, @Matovidlo - how is this handled when we have old stored events and the schema gets updated? Will the missing values be adjusted, or should I make the field optional for backward compatibility?

@tomasfejfar
Copy link
Contributor

tomasfejfar commented Sep 4, 2025

The schema validates input in postMessage endpoint in connection. So if you update it and make it required, any request to create message without the serverTransport field will fail (HTTP 400). Normally that's OK, because we only update it after we release new version of the service that produces them. With MCP it's more complicated because we don't own the deployments (people have it potentially deployed locally, not updated).

@mariankrotil
Copy link
Contributor Author

@tomasfejfar In that case, we should keep the field optional, right?

@mariankrotil
Copy link
Contributor Author

mariankrotil commented Sep 4, 2025

@tomasfejfar The property is now optional and it should work with previous versions of mcp smoothly.

@tomasfejfar tomasfejfar merged commit e82ef16 into keboola:main Sep 4, 2025
@ujovlado
Copy link
Member

ujovlado commented Sep 6, 2025

@tomasfejfar @mariankrotil

Why we kept both? Schemas for component keboola.mcp-server.tool are no longer needed, once this PR is released: keboola/mcp-server#251

@tomasfejfar
Copy link
Contributor

If we want this fixed, let's do that in a new PR and new connection release. There is no problem releasing both schemas changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants