Enhanced server.json validation (phase 1) #636
Conversation
|
There is an issue to consider with this PR, which is that I am embedding schema.server.json and using it to determine the current version string. This is in conflict (potentially) with the hard coded model CurrentSchemaVersion and CurrentSchemaURL. If we go with the embedded schema approach and we have a workflow that assures the correct schema file is used, we should use that as the source of truth and remove the hardcoded values (IMO). Since they're used extremely widely throughout the codebase, I didn't feel comfortable making that change in this PR. |
There was a problem hiding this comment.
I think this all makes sense and I'm inclined to approve as-written + let you drive the validation roadmap here onward 🚀 Just want to hear from @rdimitrov and/or @domdomegg if they are aligned with the general approach
Edit: Thought on it some more and I think I do have one fundamental gripe here -- to align with the core MCP spec, we might want to have TypeScript as a source of truth rather than JSON schema. This actually might not really impact this PR (except the design doc), but it would introduce another step we should do soon - especially before trying to expand the JSON schema - of creating the TypeScript types source of truth.
| @@ -0,0 +1,653 @@ | |||
| # Enhanced Server Validation Design | |||
|
|
|||
| NOTE: This document describes a proposed direction for improving validation of server.json data in the Official Registry. This work is in progress (including open PRs ands discussions)in a collaborative process and may change signficianty or be abandoned. | |||
There was a problem hiding this comment.
| NOTE: This document describes a proposed direction for improving validation of server.json data in the Official Registry. This work is in progress (including open PRs ands discussions)in a collaborative process and may change signficianty or be abandoned. | |
| NOTE: This document describes a proposed direction for improving validation of server.json data in the Official Registry. This work is in progress (including open PRs and discussions) in a collaborative process and may change signficiantly or be abandoned. |
|
|
||
| This document outlines the design for implementing comprehensive server validation in the MCP Registry, due to the following concerns: | ||
|
|
||
| - Currently, the MPC Registry project publishes a server.json schema but does not validate servers against it, allowing non-compliant servers to be published. |
There was a problem hiding this comment.
| - Currently, the MPC Registry project publishes a server.json schema but does not validate servers against it, allowing non-compliant servers to be published. | |
| - Currently, the MCP Registry project publishes a server.json schema but does not validate servers against it, allowing non-compliant servers to be published. |
|
@claude review this PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
tadasant
left a comment
tadasant
left a comment
There was a problem hiding this comment.
Had Claude review locally:
Claude's review
Now I have enough context to provide a comprehensive review. Here's the markdown review:
PR #636 Review: Enhanced server.json validation (phase 1)
Summary
This PR adds schema validation and improves the validation infrastructure for server.json files. The main changes include:
- A new
mcp-publisher validatecommand - JSON schema validation using embedded schema files
- Exhaustive validation that collects all errors instead of failing fast
- Rich validation results with JSON paths and references
Overall Assessment
Recommendation: Request Changes - The PR has solid architectural foundations but needs refinement before merging.
What's Good 👍
1. Well-designed validation types
The ValidationIssue and ValidationResult types are clean and well-structured:
type ValidationIssue struct {
Type ValidationIssueType
Path string
Message string
Severity ValidationIssueSeverity
Reference string
}2. Backward compatibility preserved
The approach of wrapping ValidateServerJSONExhaustive with the existing ValidateServerJSON signature is smart:
func ValidateServerJSON(serverJSON *apiv0.ServerJSON) error {
result := ValidateServerJSONExhaustive(serverJSON, false)
// Returns first error, maintaining existing behavior
}3. Immutable context building
The ValidationContext pattern for tracking JSON paths is elegant:
ctx.Field("packages").Index(0).Field("transport") // → "packages[0].transport"4. Comprehensive test coverage
The tests cover multiple error scenarios, path verification, and schema resolution.
5. Thorough design documentation
The proposed-enhanced-validation.md doc is exceptionally detailed and shows careful thought about the problem space.
Issues to Address
🔴 Critical
1. Schema embedding approach is fragile
The current approach copies the schema file during build (make prep-schema), which creates several problems:
- CI/CD complexity: Requires adding
make prep-schemato CI workflow - Developer friction: Easy to forget, causing confusing
go:embederrors - Gitignored generated files: The schema is in
.gitignorebut required for compilation
Suggestion: Consider one of these alternatives:
- Check in the copied schema file and update it via a dedicated script/workflow
- Use
go generatewith a clear directive at the top ofschema.go - Fetch the schema at runtime (with caching) rather than embedding
2. Panic in production code
In cmd/publisher/commands/init.go:
currentSchema, err := validators.GetCurrentSchemaVersion()
if err != nil {
// Should never happen (schema is embedded)
panic(fmt.Sprintf("failed to get embedded schema version: %v", err))
}Never use panic in user-facing code. Even "impossible" errors should return gracefully:
if err != nil {
return apiv0.ServerJSON{}, fmt.Errorf("internal error: failed to get schema version: %w", err)
}3. Duplicate schema version checking logic
Schema version validation appears in three places with slightly different implementations:
cmd/publisher/commands/publish.gocmd/publisher/commands/validate.gointernal/validators/validators.go(inValidateServerJSONExhaustive)
This violates DRY and risks inconsistent behavior. The validate command and publish command should both delegate to a single source of truth.
🟡 Important
4. Inconsistent path format between schema and semantic validation
Schema validation produces paths like packages.0.transport while semantic validation uses packages[0].transport. This inconsistency will confuse users.
From the test:
assert.True(t, issuePaths["packages.0.packageArguments.0.format"]) // Schema style
assert.True(t, issuePaths["packages[0].version"]) // Semantic styleFix: Normalize all paths to use the bracket notation for array indices.
5. Missing --help flag handling in validate command
func ValidateCommand(args []string) error {
for _, arg := range args {
if !strings.HasPrefix(arg, "-") {
serverFile = arg
}
}
// No handling for --help
}Other commands likely have --help support. This should too.
6. Error message still references model.CurrentSchemaVersion
The semantic validator still uses model.CurrentSchemaVersion:
case !strings.Contains(serverJSON.Schema, model.CurrentSchemaVersion):But the CLI commands now use validators.GetCurrentSchemaVersion(). If these ever diverge, you'll have inconsistent validation.
7. Schema validation errors for discriminated unions are noisy
As noted in the PR description, anyOf transport validation produces confusing errors (one error per possible type). The PR acknowledges this but doesn't fix it. Consider:
- Filing a follow-up issue to track this
- Adding a comment in the code explaining the limitation
🟢 Minor/Suggestions
8. Consider structured logging for validation results
The validate command uses fmt.Fprintf with emojis. For programmatic use, consider adding a --json flag to output structured validation results.
9. Test file organization
validation_detailed_test.go is in validators_test package (external test), while existing tests appear to be internal. Consider consistency.
10. Reference format is verbose
Reference: #/definitions/Repository/properties/url/format from: [#/definitions/ServerDetail]/properties/repository/[#/definitions/Repository]/properties/url/format
This is very long. Consider:
- Truncating for display, full in
--verbosemode - Just showing the final constraint:
#/definitions/Repository/properties/url/format
11. Missing newline at end of .gitignore
# Generated schema directory for embedding
internal/validators/schema/
No trailing newline - minor but can cause issues with some tools.
Questions for Discussion
-
Schema versioning strategy: When the embedded schema is updated, how will you ensure
model.CurrentSchemaVersionstays in sync? Should there be a single source of truth? -
Linter validation timing: The design doc mentions linter validation as "phase 2". Is there a tracking issue for this?
-
Performance impact: Has exhaustive validation been benchmarked against the fail-fast approach? For large
server.jsonfiles with many packages, this could be noticeably slower. -
Breaking change path: The PR mentions eventually removing redundant semantic validators. How will you handle the transition without breaking existing error message expectations in downstream code?
Files Changed Summary
| File | Assessment |
|---|---|
internal/validators/schema.go |
Good implementation, but embedding strategy needs work |
internal/validators/validation_types.go |
Clean, well-designed types |
internal/validators/validators.go |
Good refactor, path inconsistency needs fixing |
cmd/publisher/commands/validate.go |
Needs --help, remove panic potential |
cmd/publisher/commands/init.go |
Remove panic |
cmd/publisher/commands/publish.go |
DRY violation with validate.go |
docs/explanations/proposed-enhanced-validation.md |
Excellent documentation |
Makefile |
Works but fragile |
.github/workflows/ci.yml |
Required workaround for embedding |
Recommended Next Steps
- Fix the critical issues (panic, schema embedding strategy)
- Normalize path formats between schema and semantic validation
- Consolidate schema version checking to a single location
- Add
--helpto validate command - Consider adding a follow-up issue for the
anyOferror noise problem
From its feedback, I think:
1. Schema embedding approach is fragile- I don't know enough about Go to have an informed opinion on this. Maybe @toby can help? Let's not let this block on merging but maybe there's a more elegantgo:generatepowered solution here for the long term we can do as a fast follow.2. Panic in production code- Reasonable feedback; let's fix3. Duplicate schema version checking logic- I assume this will be made DRY in phase 2+, right? Can ignore.5. Missing--helpflag handling in validate command- worth adding?6. Error message still referencesmodel.CurrentSchemaVersion`` - worth fixing I think?
Besides that, my only feedback is a few inline comments (particularly on not using the draft version) -- otherwise basically looks good to merge! Thanks so much for all the work you've put into this.
| # Preparation targets | ||
| prep-schema: ## Copy schema file for embedding | ||
| @mkdir -p internal/validators/schema | ||
| @cp docs/reference/server-json/server.schema.json internal/validators/schema/server.schema.json |
There was a problem hiding this comment.
Should we lock this into being a static, live version of the schema (instead of this linked one, which is a draft unreleased version)?
I think in general it would be smart to not make changes in the registry codebase that don't correspond to an already-published server.json schema, so I don't think that constraint would hold us back would it?
| registry | ||
| registry | ||
|
|
||
| # Generated schema directory for embedding |
There was a problem hiding this comment.
"embedding" as a term is a little misdirecting at first glance (given the prominence of vector embeddings in AI); wherever you use this can we please make clear it's about "embedding the static file into the Go binary"
| run: | | ||
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.4.0 | ||
| - name: Prepare schema for embedding |
There was a problem hiding this comment.
Same comment as below on "embedding"
| } | ||
|
|
||
| // Check for deprecated schema and recommend migration | ||
| // Allow empty schema (will use default) but reject old schemas |
There was a problem hiding this comment.
Allowing empty schema seems weird here - do you agree? Realize it's out of scope but if you agree I'll file an Issue
2 participants
Added schema validation and support for exhaustive and more detailed validation to existing validators. Added new
mcp-publisher validatecommand. No change to any existing tests or behavior other than the new command.Motivation and Context
Many currently published servers fail schema validation. Of those that pass, many have semantic/logical errors or other errors that make them impossible to configure, or their configuration when applied doesn't generate correct MCP server configuration (per their own documentation).
To address this, we need better tooling to validate servers and to inform server publishers of errors, issues, and best practices in a way that is clear and actionable, both during server development and at time of publishing.
I have started a discussion about the broader topic: #635
There is also a fairly detailed document describing the design, architecture, implementation, future plans, etc. See Enhanced Validation Design
This PR is the first step in the process of improving validation. It adds schema validation and updates all existing validators to use a new model that allows them to track context and return rich and exhaustive results. This has been done in a way that is backward compatible (ValidateServerJSON is unchanged as are all existing validation unit tests).
There is a new
mcp-publishercommand calledvalidatethat is currenty the only place that schema validation and enhanced (exhaustive/rich) validation results are exposed.Changes
Internal Validation improvements
A new
validatecommand has been added tomcp-publisherso publishers can evaluate their server.json before publishingUsage
Given a server.json that looks like this:
{ "$schema": "https://static.modelcontextprotocol.io/schemas/2025-09-29/server.schema.json", "name": "io.github.BobDickinson/registry", "description": "An MCP server that provides [describe what your server does]", "repository": { "url": "", "source": "" }, "version": "=1.0.0", "packages": [ { "registryType": "oci", "registryBaseUrl": "https://docker.io", "identifier": "registry", "version": "1.0.0", "transport": { "type": "sse" }, "packageArguments": [ { "type": "named", "name": "--mode", "description": "Operation mode", "format": "foo" } ], "environmentVariables": [ { "description": "Your API key for the service", "isRequired": true, "format": "string", "isSecret": true, "name": "YOUR_API_KEY" } ] } ] }Run the command:
mcp-publisher validate server.jsonWhich will produce the output:
Note in the results above:
Referenceprovided contains an absolute path to the schema rule that was violated, followed by the full schema path that enforced that rule (with $ref redirect values substited and enclosed in square brackets).In the final solution we would remove semantic validators that are redundant to schema validation (as in number 5 above). We have the option to use the structured data produced by schema validation to replace the generic schema validation message with a more descriptive message if that is an issue (in particular, if the only argument for an ad-hoc validator is that is produces a better/cleaner message, we would just move that message creation code to the schema error result formatter and special case it as opposed to having a redundant validator).
What's Next
If this general direction is supported and this PR is accepted, a fast follow would include:
Issues
Schema validation requires embedding the
server.schema.jsonfile into the validators package viago:embed, which is restricted from embedding files outside of the package. For this reason we copy the schema file into a schema subdir (via a prep target in the makefile) and we .gitignore it. I tried cleaning up the copy in the make clean target, but then the linter would complain about the embed target being missing if it wasn't there during development. I also considered just checking in the schema file and assuming a separate workflow for updating the embedded schema file, but it's not clear what that workflow would be or how it would be maintained. I'm open to a different approach, but the schema file does need to be embedded somehow for schema validation to work.How Has This Been Tested?
All existing validation tests pass, new tests implemented and all pass.
Breaking Changes
No breaking changes, no behavior change except new
validatecommandTypes of changes
Checklist