- Notifications
You must be signed in to change notification settings - Fork 156
Use standard JSON Schema format for VMCP composite tool parameters #2651
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
Conversation
3b5b3da to b88bc23 Compare Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #2651 +/- ## ========================================== + Coverage 56.34% 56.45% +0.11% ========================================== Files 319 319 Lines 30887 30878 -9 ========================================== + Hits 17402 17431 +29 + Misses 11999 11948 -51 - Partials 1486 1499 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I added some nits. Only because I was reviewing the docs earlier this week do I know that we in fact have docs (docs/operator/advanced-workflow-patterns.md and docs/operator/virtualmcpcompositetooldefinition-guide.md) that @tgrunnagle wrote, thos should probably be updated as well
b88bc23 to 95d49b8 Compare
tgrunnagle left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions but non-blocking
95d49b8 to c9e1a4c Compare | Thanks for the review @tgrunnagle! Regarding your questions: Strong typing for Parameters (config.go) Non-object top level parameters (yaml_loader.go) |
c9e1a4c to 60bdf08 Compare 60bdf08 to 392701a Compare There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification [Explain why this PR must be large, such as:] - Generated code that cannot be split - Large refactoring that must be atomic - Multiple related changes that would break if separated - Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Fixes #2650 Changes composite tool parameter definitions to use standard JSON Schema format per the MCP specification, instead of a non-standard simplified format. **Breaking Change**: Composite tool parameter format has changed from: ```yaml parameters: param1: type: "string" default: "value" ``` To standard JSON Schema: ```yaml parameters: type: object properties: param1: type: string default: "value" required: ["param1"] ``` Changes: - Update CompositeToolConfig.Parameters to map[string]any (full JSON Schema) - Remove parameter transformation logic from yaml_loader and workflow_converter - Add JSON Schema validation in yaml_loader (checks type: object) - Update Kubernetes CRDs to use runtime.RawExtension for parameters - Update webhook validation to handle runtime.RawExtension - Regenerate CRD manifests and deepcopy code - Update all tests and documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
392701a to 70b7945 Compare it's no longer as big as it used to be
Add explicit test case to converter_test.go that verifies description and default fields at the parameter property level are correctly preserved when using JSON Schema format. This test documents that issue #2775 (field mismatch between ParameterSpec and ParameterSchema) is resolved by the JSON Schema approach, which supports all JSON Schema fields automatically. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add explicit test cases verifying that JSON Schema fields (description, default, required) are preserved throughout the entire conversion chain: 1. converter_test.go: Tests CRD → config conversion preserves fields 2. capability_adapter_test.go: Tests config → MCP SDK conversion preserves fields These tests document that issue #2775 (field mismatch between ParameterSpec and ParameterSchema) is fully resolved by the JSON Schema approach, which passes the entire schema through without any field stripping. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixes #2650
Fixes #2775
Summary
Changes composite tool parameter definitions to use standard JSON Schema format per the MCP specification, instead of a non-standard simplified format.
This also resolves the field mismatch between
ParameterSpec(CRD) andParameterSchema(implementation) documented in #2775. By moving to standard JSON Schema format withmap[string]any, all JSON Schema fields (description,required,default, etc.) are now supported automatically without needing separate struct fields.Large PR Justification
This PR exceeds 1000 lines due to the combination of:
description,default,requiredfield preservation)The changes are tightly coupled and cannot be safely split - a partial migration would leave the system in an inconsistent state where some components expect the old format and others expect JSON Schema.
Breaking Change
Composite tool parameter format has changed from:
To standard JSON Schema:
Changes
CompositeToolConfig.Parameterstomap[string]any(full JSON Schema)ParameterSchemastruct from implementation (fixes Align ParameterSpec and ParameterSchema fields between CRD and implementation #2775)ParameterSpectype from CRD, replaced withruntime.RawExtensionruntime.RawExtensionfor parametersruntime.RawExtensionTest plan
descriptionanddefaultfields preservation (issue Align ParameterSpec and ParameterSchema fields between CRD and implementation #2775)🤖 Generated with Claude Code