diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index be7ccf9e..8b1eef30 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,6 +40,9 @@ jobs: 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 + run: make prep-schema + - name: Run lint run: make lint diff --git a/.gitignore b/.gitignore index 8d3145ac..526b20e4 100644 --- a/.gitignore +++ b/.gitignore @@ -12,4 +12,7 @@ validate-schemas coverage.out coverage.html deploy/infra/infra -registry \ No newline at end of file +registry + +# Generated schema directory for embedding +internal/validators/schema/ \ No newline at end of file diff --git a/Makefile b/Makefile index dc08004e..206688e7 100644 --- a/Makefile +++ b/Makefile @@ -5,12 +5,17 @@ help: ## Show this help message @echo "Available targets:" @grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf " %-20s %s\n", $$1, $$2}' +# 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 + # Build targets -build: ## Build the registry application with version info +build: prep-schema ## Build the registry application with version info @mkdir -p bin go build -ldflags="-X main.Version=dev-$(shell git rev-parse --short HEAD) -X main.GitCommit=$(shell git rev-parse HEAD) -X main.BuildTime=$(shell date -u +%Y-%m-%dT%H:%M:%SZ)" -o bin/registry ./cmd/registry -publisher: ## Build the publisher tool with version info +publisher: prep-schema ## Build the publisher tool with version info @mkdir -p bin go build -ldflags="-X main.Version=dev-$(shell git rev-parse --short HEAD) -X main.GitCommit=$(shell git rev-parse HEAD) -X main.BuildTime=$(shell date -u +%Y-%m-%dT%H:%M:%SZ)" -o bin/mcp-publisher ./cmd/publisher @@ -26,7 +31,7 @@ check-schema: ## Check if server.schema.json is in sync with openapi.yaml @./bin/extract-server-schema -check # Test targets -test-unit: ## Run unit tests with coverage (requires PostgreSQL) +test-unit: prep-schema ## Run unit tests with coverage (requires PostgreSQL) @echo "Starting PostgreSQL for unit tests..." @docker compose up -d postgres @echo "Waiting for PostgreSQL to be ready..." diff --git a/cmd/publisher/commands/init.go b/cmd/publisher/commands/init.go index 6b2ac27a..fc7610a8 100644 --- a/cmd/publisher/commands/init.go +++ b/cmd/publisher/commands/init.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/modelcontextprotocol/registry/internal/validators" apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0" "github.com/modelcontextprotocol/registry/pkg/model" ) @@ -300,8 +301,15 @@ func createServerJSON( } // Create server structure + // Get current schema version from embedded schema + currentSchema, err := validators.GetCurrentSchemaVersion() + if err != nil { + // Should never happen (schema is embedded) + panic(fmt.Sprintf("failed to get embedded schema version: %v", err)) + } + return apiv0.ServerJSON{ - Schema: model.CurrentSchemaURL, + Schema: currentSchema, Name: name, Description: description, Repository: model.Repository{ diff --git a/cmd/publisher/commands/publish.go b/cmd/publisher/commands/publish.go index 6a42c943..2138c3ec 100644 --- a/cmd/publisher/commands/publish.go +++ b/cmd/publisher/commands/publish.go @@ -12,8 +12,8 @@ import ( "path/filepath" "strings" + "github.com/modelcontextprotocol/registry/internal/validators" apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0" - "github.com/modelcontextprotocol/registry/pkg/model" ) func PublishCommand(args []string) error { @@ -40,13 +40,24 @@ func PublishCommand(args []string) error { // Check for deprecated schema and recommend migration // Allow empty schema (will use default) but reject old schemas - if serverJSON.Schema != "" && !strings.Contains(serverJSON.Schema, model.CurrentSchemaVersion) { - return fmt.Errorf(`deprecated schema detected: %s. + if serverJSON.Schema != "" { + // Get current schema version from embedded schema + currentSchema, err := validators.GetCurrentSchemaVersion() + if err != nil { + // Schema is embedded, so this should never happen + return fmt.Errorf("failed to get current schema version: %w", err) + } + + if serverJSON.Schema != currentSchema { + return fmt.Errorf(`deprecated schema detected: %s. + +Expected current schema: %s Migrate to the current schema format for new servers. 📋 Migration checklist: https://github.com/modelcontextprotocol/registry/blob/main/docs/reference/server-json/CHANGELOG.md#migration-checklist-for-publishers -📖 Full changelog with examples: https://github.com/modelcontextprotocol/registry/blob/main/docs/reference/server-json/CHANGELOG.md`, serverJSON.Schema) +📖 Full changelog with examples: https://github.com/modelcontextprotocol/registry/blob/main/docs/reference/server-json/CHANGELOG.md`, serverJSON.Schema, currentSchema) + } } // Load saved token diff --git a/cmd/publisher/commands/validate.go b/cmd/publisher/commands/validate.go new file mode 100644 index 00000000..d1ecb6ba --- /dev/null +++ b/cmd/publisher/commands/validate.go @@ -0,0 +1,81 @@ +package commands + +import ( + "encoding/json" + "fmt" + "os" + "strings" + + "github.com/modelcontextprotocol/registry/internal/validators" + apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0" +) + +func ValidateCommand(args []string) error { + // Parse arguments + serverFile := "server.json" + + for _, arg := range args { + if !strings.HasPrefix(arg, "-") { + serverFile = arg + } + } + + // Read server file + serverData, err := os.ReadFile(serverFile) + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("%s not found, please check the file path", serverFile) + } + return fmt.Errorf("failed to read %s: %w", serverFile, err) + } + + // Validate JSON + var serverJSON apiv0.ServerJSON + if err := json.Unmarshal(serverData, &serverJSON); err != nil { + return fmt.Errorf("invalid JSON: %w", err) + } + + // Check for deprecated schema and recommend migration + // Allow empty schema (will use default) but reject old schemas + if serverJSON.Schema != "" { + // Get current schema version from embedded schema + currentSchema, err := validators.GetCurrentSchemaVersion() + if err != nil { + // Should never happen (schema is embedded) + return fmt.Errorf("failed to get current schema version: %w", err) + } + + if serverJSON.Schema != currentSchema { + return fmt.Errorf(`deprecated schema detected: %s. + +Expected current schema: %s + +Migrate to the current schema format for new servers. + +📋 Migration checklist: https://github.com/modelcontextprotocol/registry/blob/main/docs/reference/server-json/CHANGELOG.md#migration-checklist-for-publishers +📖 Full changelog with examples: https://github.com/modelcontextprotocol/registry/blob/main/docs/reference/server-json/CHANGELOG.md`, serverJSON.Schema, currentSchema) + } + } + + // Run detailed validation (this is the whole point of the validate command) + // Include schema validation for comprehensive validation + result := validators.ValidateServerJSONExhaustive(&serverJSON, true) + + if result.Valid { + _, _ = fmt.Fprintln(os.Stdout, "✅ server.json is valid") + return nil + } + + // Print all issues + _, _ = fmt.Fprintf(os.Stdout, "❌ Validation failed with %d issue(s):\n\n", len(result.Issues)) + for i, issue := range result.Issues { + _, _ = fmt.Fprintf(os.Stdout, "%d. [%s] %s (%s)\n", i+1, issue.Severity, issue.Path, issue.Type) + _, _ = fmt.Fprintf(os.Stdout, " %s\n", issue.Message) + if issue.Reference != "" { + _, _ = fmt.Fprintf(os.Stdout, " Reference: %s\n", issue.Reference) + } + _, _ = fmt.Fprintln(os.Stdout) + } + + return fmt.Errorf("validation failed") +} diff --git a/cmd/publisher/main.go b/cmd/publisher/main.go index db0ef924..6467263a 100644 --- a/cmd/publisher/main.go +++ b/cmd/publisher/main.go @@ -37,6 +37,8 @@ func main() { err = commands.LogoutCommand() case "publish": err = commands.PublishCommand(os.Args[2:]) + case "validate": + err = commands.ValidateCommand(os.Args[2:]) case "--version", "-v", "version": log.Printf("mcp-publisher %s (commit: %s, built: %s)", Version, GitCommit, BuildTime) return @@ -65,6 +67,7 @@ func printUsage() { _, _ = fmt.Fprintln(os.Stdout, " login Authenticate with the registry") _, _ = fmt.Fprintln(os.Stdout, " logout Clear saved authentication") _, _ = fmt.Fprintln(os.Stdout, " publish Publish server.json to the registry") + _, _ = fmt.Fprintln(os.Stdout, " validate Validate server.json without publishing") _, _ = fmt.Fprintln(os.Stdout) _, _ = fmt.Fprintln(os.Stdout, "Use 'mcp-publisher --help' for more information about a command.") } diff --git a/docs/explanations/proposed-enhanced-validation.md b/docs/explanations/proposed-enhanced-validation.md new file mode 100644 index 00000000..8863c7ae --- /dev/null +++ b/docs/explanations/proposed-enhanced-validation.md @@ -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. + +## Overview + +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 is existing ad-hoc validation that covers some schema compliance, but not all (there are logical errors not identifiable by schema validation and that are not covered by the existing ad hoc validation). +- Many servers that do pass validation do not represent best-practices for published servers. + +This design implements a three-tier validation system: **Schema Validation**, **Semantic Validation**, and **Linter Validation**. + +## Current State + +### Problems with Current Validation +- **No schema validation**: Servers are published without validating against the published schema (and many violate it) +- **Incomplete validation**: Ad hoc validation covers only some schema constraints (many published servers have additional logical errors) +- **Best Practices not indicated**: Many servers that would pass schema and semantic validation do not represent best practices +- **Fail-fast behavior**: `ValidateServerJSON()` stops at first error +- **No path information**: Errors don't specify where in JSON the problem occurs + +## Three-Tier Validation System + +### Schema Validation (Primary) +- **Validates against published schema**: Ensures servers comply with the official server.json schema +- **Exhaustive coverage**: Catches all structural and format violations defined in the schema +- **Detailed error references**: Shows exact schema rule locations with specific constraint and full path to constraint + +### Semantic Validation (Secondary) +- **Business logic validation**: Validates only constraints not expressible in JSON Schema +- **Registry validation**: Enforce validitiy of registry references (as current) +- **Logical Errors**: Enforce logical consistency: format, choices, variable usage, etc + +### Linter Validation (Tertiary) +- **Best practice recommendations**: Security concerns, style guidelines, naming conventions +- **Non-blocking**: Warnings and suggestions, not errors +- **Quality improvements**: Helps developers create better servers +- **Educational**: Teaches best practices for MCP server development + +## Implementation Approach + +The enhanced validation will be implemented in stages to minimize risk and allow for review and experimentation: + +### **Stage 1: Schema Validation and Exhaustive Validation Results (Current)** +- Convert existing validators to use and track context and to return exhaustive results +- Add `mcp-publisher validate` command that performs exhaustive validation +- Implement schema validation but only enable it for the `validate` command (not the `/v0/publish` API) +- Maintain backward compatibility with no production impact + - All existing validation calls use a wrapper that returns the first error + - Existing validation tests work without modification (since they call the wrapper) +- This allows experimentation and validation of the new model (including schema validation) without impacting production code + +### **Future Stages** +- Enable schema validation in all validation cases (including the `/v0/publish` API endpoint) - flip boolean switch +- Build out comprehensive semantic and linter validation rules (with tests) +- Remove redundant manual validators that duplicate schema constraints +- Update unit tests to handle rich/exhaustive validation results + +## Proposed Design + +### Design Goals + +1. **Exhaustive Feedback**: Collect all validation issues in a single pass, not just the first error +2. **Precise Location**: Provide exact JSON paths for every validation issue +3. **Structured Output**: Return machine-readable validation results with consistent format +4. **Backward Compatibility**: Maintain existing `ValidateServerJSON() error` signature +5. **Extensible**: Support different validation types (json, schema, semantic, linter) and severity levels + + +### Core Types + +```go +// Validation issue type with constrained values +type ValidationIssueType string + +const ( + ValidationIssueTypeJSON ValidationIssueType = "json" + ValidationIssueTypeSchema ValidationIssueType = "schema" + ValidationIssueTypeSemantic ValidationIssueType = "semantic" + ValidationIssueTypeLinter ValidationIssueType = "linter" +) + +// Validation issue severity with constrained values +type ValidationIssueSeverity string + +const ( + ValidationIssueSeverityError ValidationIssueSeverity = "error" + ValidationIssueSeverityWarning ValidationIssueSeverity = "warning" + ValidationIssueSeverityInfo ValidationIssueSeverity = "info" +) + +type ValidationIssue struct { + Type ValidationIssueType `json:"type"` + Path string `json:"path"` // JSON path like "packages[0].transport.url" + Message string `json:"message"` // Error description (extracted from error.Error()) + Severity ValidationIssueSeverity `json:"severity"` + Reference string `json:"reference"` // Schema rule path or rule name like "prefer-transport-configuration" +} + +type ValidationResult struct { + Valid bool `json:"valid"` + Issues []ValidationIssue `json:"issues"` +} + +type ValidationContext struct { + path string +} + +// Constructor functions following Go conventions +func NewValidationIssue(issueType ValidationIssueType, path, message string, severity ValidationIssueSeverity, reference string) ValidationIssue +func NewValidationIssueFromError(issueType ValidationIssueType, path string, err error, reference string) ValidationIssue +``` + +### Validation Types + +The `Type` field categorizes validation issues by their source: + +- **`ValidationIssueTypeJSON`**: JSON parsing errors (malformed JSON syntax) +- **`ValidationIssueTypeSchema`**: JSON Schema validation errors (structural/format violations) +- **`ValidationIssueTypeSemantic`**: Logical validation errors not enforceable in schema (business rules) +- **`ValidationIssueTypeLinter`**: Best practice recommendations, security concerns, style guidelines + +The `Severity` field indicates the impact level: + +- **`ValidationIssueSeverityError`**: Critical issues that must be fixed +- **`ValidationIssueSeverityWarning`**: Issues that should be addressed +- **`ValidationIssueSeverityInfo`**: Suggestions and recommendations + +The `Reference` field provides context about what triggered the validation issue: + +- **Schema validation**: Contains the resolved schema path with `$ref` resolution (e.g., `"#/definitions/SseTransport/properties/url/format from: [#/definitions/ServerDetail]/properties/packages/items/[#/definitions/Package]/properties/transport/properties/url/format"`) +- **Semantic validation**: Contains rule names for business logic (e.g., `"invalid-server-name"`, `"missing-transport-url"`) +- **Linter validation**: Contains rule names for best practices (e.g., `"descriptive-naming"`, `"security-recommendation"`) +- **JSON validation**: Contains error type identifiers (e.g., `"json-syntax-error"`, `"invalid-json-format"`) + +### ValidationContext + +The `ValidationContext` tracks the current JSON path during validation, allowing validators to report issues with precise location information. This is essential for providing users with exact paths to problematic fields. + +#### **Purpose** +- **Path tracking**: Builds JSON paths like `"packages[0].transport.url"` as validation traverses nested structures +- **Precise error location**: Users can see exactly where validation issues occur +- **Immutable building**: Each method returns a new context, preventing accidental mutations + +#### **Usage Example** +```go +// Navigate to packages array, first item, transport field +pkgCtx := ctx.Field("packages").Index(0).Field("transport") +// Validate transport - any issues will be reported at "packages[0].transport" +``` + +### Backward Compatibility Strategy + +The design maintains perfect backward compatibility by leveraging Go's existing error handling patterns: + +#### **Error Message Preservation** +- **Current validators** use `fmt.Errorf("%w: %s", ErrInvalidRepositoryURL, obj.URL)` +- **New validators** use `NewValidationIssueFromError()` which extracts `err.Error()` +- **Result**: Identical error messages, ensuring all existing tests pass + +#### **Constructor Pattern** +Following Go conventions used throughout the project: +```go +// Standard constructor for manual field setting +issue := NewValidationIssue(ValidationIssueTypeLinter, "name", "message", ValidationIssueSeverityWarning, "rule-name") + +// Constructor that preserves existing error formatting +issue := NewValidationIssueFromError(ValidationIssueTypeSemantic, "path", err, "rule-name") +``` + +#### **Error Interface Compatibility** +- Existing `ValidateServerJSON() error` signature unchanged +- Returns `fmt.Errorf("%s", issue.Message)` - same string format +- All `errors.Is()` and `errors.As()` calls continue to work +- No changes needed to error handling code + +### New Validation Architecture + +#### **All Validators Use Context and Return ValidationResult** + +All existing validators are converted to use `ValidationContext` for precise error location tracking and return `ValidationResult` for comprehensive error collection: + +```go +func ValidateServerJSONExhaustive(serverJSON *apiv0.ServerJSON) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + + // Validate server name - using existing error logic + if _, err := parseServerName(*serverJSON); err != nil { + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, // All existing validation uses "semantic" type + "name", + err, // Preserves existing error formatting + "invalid-server-name", + ) + result.AddIssue(issue) + } + + // Validate repository with context + if repoResult := validateRepository(&ValidationContext{}, &serverJSON.Repository); !repoResult.Valid { + result.Merge(repoResult) + } + + // Validate packages with array context + for i, pkg := range serverJSON.Packages { + pkgCtx := &ValidationContext{}.Field("packages").Index(i) + if pkgResult := validatePackageField(pkgCtx, &pkg); !pkgResult.Valid { + result.Merge(pkgResult) + } + } + + // Validate remotes with array context + for i, remote := range serverJSON.Remotes { + remoteCtx := &ValidationContext{}.Field("remotes").Index(i) + if remoteResult := validateRemoteTransport(remoteCtx, &remote); !remoteResult.Valid { + result.Merge(remoteResult) + } + } + + return result +} +``` + +#### **Existing Validator Becomes Simple Wrapper** + +```go +func ValidateServerJSON(serverJSON *apiv0.ServerJSON) error { + result := ValidateServerJSONExhaustive(serverJSON) + if !result.Valid { + // Return the first error-level issue + for _, issue := range result.Issues { + if issue.Severity == "error" { + return fmt.Errorf("%s: %s", issue.Path, issue.Message) + } + } + } + return nil +} +``` + +## Schema Validation + +The project uses `github.com/santhosh-tekuri/jsonschema/v5` for schema validation with an embedded schema approach. The schema is embedded at compile time using Go's `//go:embed` directive, eliminating the need for file system access and ensuring the schema is always available. + +### Schema-First Validation Strategy + +The enhanced validation system adopts a **schema-first approach** where JSON Schema validation serves as the primary and first validator. This strategy addresses the current duplication between manual/semantic validators and schema constraints. + +#### **Current Problem: Validation Duplication** + +The existing system has both: +- **Manual/semantic validators**: Custom Go code validating server name format, URL patterns, etc. +- **JSON Schema validation**: Structural validation of the same constraints + +This creates redundancy and potential inconsistencies where: +- Manual validators provide friendly error messages +- Schema validation provides technical error messages +- Both validate the same underlying constraints + +#### **Proposed Solution: Schema-First with Friendly Error Mapping** + +1. **Schema validation runs first** and catches all structural/format issues +2. **Manual validators are eliminated** for constraints already specified in the schema +3. **Schema error messages are mapped to friendly messages** using deterministic schema rule references (if needed) + +### Embedded Schema Benefits + +#### **No File System Dependencies** +- **Embedded at compile time**: Schema is included in the binary using `//go:embed schema/*.json` +- **No external files**: Eliminates dependency on schema files being present at runtime +- **Portable**: Binary contains everything needed for validation + +#### **Version Consistency** +- **Schema version tracking**: `GetCurrentSchemaVersion()` extracts the `$id` field from embedded schema +- **Compile-time validation**: Schema is validated when the binary is built +- **No version drift**: Schema version is locked to the binary version + +#### **Performance Benefits** +- **No I/O operations**: Schema is already in memory +- **Faster startup**: No need to read schema files +- **Reduced complexity**: No file path resolution or error handling for missing files + +### Rich Error Information + +The `jsonschema.ValidationError` provides: +- **InstanceLocation**: JSON path to the invalid field (e.g., `"/packages/0/transport/url"`) +- **Error**: Detailed error message from schema +- **KeywordLocation**: Schema path with $ref segments (e.g., `"/$ref/properties/transport/$ref/properties/url/format"`) +- **AbsoluteKeywordLocation**: Resolved schema path (e.g., `"file:///server.schema.json#/definitions/SseTransport/properties/url/format"`) + +#### **Current Error Reference Format** + +Schema validation errors now include detailed reference information: + +``` +Reference: #/definitions/Repository/properties/url/format from: [#/definitions/ServerDetail]/properties/repository/[#/definitions/Repository]/properties/url/format +``` + +This format provides: +- **Absolute location**: `#/definitions/Repository/properties/url/format` - the final resolved schema location +- **Resolved path**: Shows the complete path with `$ref` segments replaced by their resolved values in square brackets +- **Full context**: Users can see exactly which schema rule triggered the error and how it was reached + +#### **Error Message Quality** + +The current schema validation errors are generally quite readable: + +``` +[error] repository.url (schema) +'' has invalid format 'uri' +Reference: #/definitions/Repository/properties/url/format from: [#/definitions/ServerDetail]/properties/repository/[#/definitions/Repository]/properties/url/format +``` + +#### **Future Error Message Enhancement** + +If we encounter situations where schema validation errors need to be more user-friendly, we have full access to: + +- **`KeywordLocation`**: The schema path to the validating rule +- **`AbsoluteKeywordLocation`**: The absolute schema location after `$ref` resolution +- **`InstanceLocation`**: The JSON path of the element that triggered the violation +- **`Message`**: The original schema validation error message +- **Complete reference stack**: The entire resolved path showing how the error was reached + +This allows us to build better, more descriptive error messages if needed, while maintaining the current high-quality error references. + +### Integration with ValidateServerJSONExhaustive + +```go +func ValidateServerJSONExhaustive(serverJSON *apiv0.ServerJSON, validateSchema bool) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + ctx := &ValidationContext{} + + // Schema validation first (if requested) - catches structural issues early + if validateSchema { + schemaResult := validateServerJSONSchema(serverJSON) + result.Merge(schemaResult) + // If schema validation fails, we might still want to run semantic validation + // to provide additional context, but schema errors take precedence + } + + // Semantic validation (always runs) - business logic not covered by schema + if _, err := parseServerName(*serverJSON); err != nil { + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("name").String(), + err, + "invalid-server-name", + ) + result.AddIssue(issue) + } + + // ... more semantic validation ... + + return result +} +``` + +### Transport Validation Improvements + +Currently the transport validation fails in a pretty ugly way (if no transport is fully satisfied, you get validation errors for all transports). The current schema is: + + "transport": { + "anyOf": [ + { + "$ref": "#/definitions/StdioTransport" + }, + { + "$ref": "#/definitions/StreamableHttpTransport" + }, + { + "$ref": "#/definitions/SseTransport" + } + ], + "description": "Transport protocol configuration for the package" + }, + +And if you have an "sse" transport with no url, you get these schema errors: + +1. [error] packages.0.transport.type (schema) + value must be "stdio" + Reference: #/definitions/StdioTransport/properties/type/enum + +2. [error] packages.0.transport (schema) + missing required fields: 'url' + Reference: #/definitions/StreamableHttpTransport/required + +3. [error] packages.0.transport.type (schema) + value must be "streamable-http" + Reference: #/definitions/StreamableHttpTransport/properties/type/enum + +4. [error] packages.0.transport (schema) + missing required fields: 'url' + Reference: #/definitions/SseTransport/require + +If we used a spec to select the discriminated type, like this: + + "transport": { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": ["stdio", "streamable-http", "sse"] + } + }, + "required": ["type"], + "if": {"properties": {"type": {"const": "stdio"}}}, + "then": {"$ref": "#/definitions/StdioTransport"}, + "else": { + "if": {"properties": {"type": {"const": "streamable-http"}}}, + "then": {"$ref": "#/definitions/StreamableHttpTransport"}, + "else": {"$ref": "#/definitions/SseTransport"} + }, + "description": "Transport protocol configuration for the package" + } + +Then it would fix on the "see" transport reference (by type) and validate against it only, producing only the single (correct) schema violation: + +1. [error] packages.0.transport (schema) + missing required fields: 'url' + Reference: #/definitions/SseTransport/required + +Same applies to Argument and remotes + +## Implementation Status + +### ✅ Completed Features + +#### **Core Validation System** +- [x] **ValidationIssue and ValidationResult types**: Complete with all required fields +- [x] **ValidationContext**: Immutable context building for JSON path tracking +- [x] **Constructor functions**: `NewValidationIssue()` and `NewValidationIssueFromError()` with consistent parameter naming +- [x] **Helper methods**: Context building, result merging, and path construction + +#### **Schema Validation Integration** +- [x] **JSON Schema validation**: Using existing `jsonschema/v5` library +- [x] **Error conversion**: Schema errors converted to `ValidationIssue` format +- [x] **$ref resolution**: Sophisticated resolution showing complete schema path with resolved references +- [x] **Comprehensive testing**: Full test coverage for schema validation scenarios +- [x] **Embedded schema**: Schema embedded at compile time using `//go:embed` directive + +#### **Enhanced Error References** +- [x] **Resolved schema paths**: Shows complete path with `$ref` segments replaced by resolved values +- [x] **Incremental resolution**: Each `$ref` resolved in context of previous resolution +- [x] **Human-readable format**: Clear indication of schema rule location and resolution chain +- [x] **Consistent output**: All schema errors use the same reference format + +#### **Testing and Quality** +- [x] **Unit tests**: Comprehensive test coverage for all new functionality +- [x] **Integration tests**: End-to-end validation testing +- [x] **Backward compatibility**: Existing validation continues to work + +### 🔄 In Progress + +#### **Schema-First Validation Strategy** +- [x] **Schema validation integration**: `ValidateServerJSONExhaustive()` runs schema validation first +- [x] **CLI integration**: Schema validation enabled in `mcp-publisher validate` command +- [ ] **Discriminated unions**: Replace `anyOf` with `if/then/else` for transport, argument, and remote validation +- [ ] **Error message mapping**: Map technical schema errors to user-friendly messages (if needed) +- [ ] **Validator migration**: Move from manual validators to schema-first approach + +### 📋 Pending + +#### **Migration Strategy** +- [ ] **Phase 1: Identify Schema Coverage**: Audit existing manual validators against schema constraints +- [ ] **Phase 2: Implement Error Mapping (Optional)**: Create mapping function for schema error messages (only if current messages are insufficient) +- [ ] **Phase 3: Enable Schema-First Validation**: Update tests to expect schema validation errors instead of semantic errors; Enable schema validation in publish API +- [ ] **Phase 4: Clean Up Redundant Validators**: Remove manual validators that duplicate schema constraints +- [ ] **Phase 5: Add Enhanced Semantic and Linter Rules**: Review and implement specific rules from [MCP Registry Validator linter guidelines](https://github.com/TeamSparkAI/ToolCatalog/blob/main/packages/mcp-registry-validator/linter.md) + +#### **Command Integration** +- [ ] **CLI updates**: Update `mcp-publisher validate` command to use detailed validation +- [ ] **Output formatting**: Add JSON output format options +- [ ] **Filtering options**: Add severity and type filtering + +#### **Documentation and Polish** +- [ ] **API documentation**: Update API documentation with new validation types + +### 🎯 Key Achievements + +1. **Comprehensive Error Collection**: All validation issues collected in single pass +2. **Precise Error Location**: Exact JSON paths for every validation issue +3. **Schema Integration**: Full JSON Schema validation with detailed error references +4. **Backward Compatibility**: Existing validation continues to work unchanged +5. **Type Safety**: Constrained types prevent invalid validation issue creation +6. **Extensible Architecture**: Easy to add new validation types and severity levels + +The enhanced validation system is now production-ready with comprehensive schema validation, detailed error references, and full backward compatibility. + + +## Example Usage + +### JSON Output Format +```json +{ + "valid": false, + "issues": [ + { + "type": "json", + "path": "", + "message": "invalid JSON syntax at line 5, column 12", + "severity": "error", + "reference": "json-syntax-error" + }, + { + "type": "semantic", + "path": "name", + "message": "server name must be in format 'dns-namespace/name'", + "severity": "error", + "reference": "invalid-server-name" + }, + { + "type": "semantic", + "path": "packages[0].transport.url", + "message": "url is required for streamable-http transport type", + "severity": "error", + "reference": "missing-transport-url" + }, + { + "type": "schema", + "path": "packages[1].environmentVariables[0].name", + "message": "string does not match required pattern", + "severity": "error", + "reference": "#/definitions/EnvironmentVariable/properties/name/pattern from: [#/definitions/ServerDetail]/properties/packages/items/[#/definitions/Package]/properties/environmentVariables/items/[#/definitions/EnvironmentVariable]/properties/name/pattern" + }, + { + "type": "linter", + "path": "packages[1].description", + "message": "consider adding a more descriptive package description", + "severity": "warning", + "reference": "descriptive-package-description" + } + ] +} +``` + +**Note**: The JSON output still uses string values for `type` and `severity` fields for JSON serialization compatibility, but the Go code uses the typed constants for type safety. + +### CLI Usage +```bash +# Basic validation +mcp-publisher validate server.json + +# JSON output format +mcp-publisher validate --format json server.json + +# Filter by severity +mcp-publisher validate --severity error server.json + +# Include schema validation +mcp-publisher validate --schema server.json +``` + +## Benefits and Achievements + +### ✅ Comprehensive Feedback +- **Exhaustive error collection**: See all validation issues at once, not just the first error +- **Better developer experience**: No need to fix errors one by one +- **Precise error location**: JSON paths show exactly where issues occur in large JSON files +- **Structured output**: JSON format for tooling integration and machine-readable error information + +### ✅ Schema-First Validation +- **Primary validator**: Schema validation catches all structural and format violations defined in the schema +- **Semantic validation only for gaps**: Covers business logic that cannot be expressed in JSON Schema +- **Standards compliance**: Ensures server.json follows the official schema +- **Detailed error messages**: Exact JSON paths and resolved schema references + +### ✅ Backward Compatibility +- **Existing `ValidateServerJSON() error` signature unchanged**: All existing code continues to work +- **Error interface compatibility**: Leverages Go's error interface and existing error constants +- **Constructor pattern**: Follows established project conventions +- **No breaking changes**: All error handling code remains functional + +### ✅ Extensible Architecture +- **Easy to add new validation types**: Schema, semantic, linter validation +- **Easy to add new severity levels**: Error, warning, info +- **Easy to add filtering and formatting options**: By type, severity, path pattern +- **Type safety**: Constrained types prevent invalid validation issue creation + +### ✅ Schema-First Strategy Benefits +- **Eliminates duplication**: Single source of truth for structural constraints +- **Better error messages**: Schema validation provides precise JSON paths with deterministic mapping +- **Maintainability**: Schema changes automatically update validation +- **Standards compliance**: Ensures validation matches official schema exactly + +## Technical Design + +### Architecture Overview + +The enhanced validation system uses a **schema-first approach** with comprehensive error collection and precise location tracking. The system is designed for maximum backward compatibility while providing extensive new capabilities. + +#### **Error Interface Compatibility** +- **Leverages existing error constants**: `ErrInvalidRepositoryURL`, `ErrVersionLooksLikeRange`, etc. +- **Preserves error wrapping**: Uses `fmt.Errorf("%w: %s", err, context)` pattern +- **Maintains error.Is() compatibility**: Existing error checking continues to work +- **No breaking changes**: All error handling code remains functional + +#### **Constructor Pattern** +Following established Go conventions in the project: +- **`NewValidationIssue()`**: Standard constructor following `NewXxx()` pattern +- **`NewValidationIssueFromError()`**: Specialized constructor for error conversion +- **Consistent with project**: Matches patterns used in `NewConfig()`, `NewServer()`, etc. +- **Type safety**: Compile-time validation of required fields + +#### **Context Passing Architecture** +- **Immutable context building**: `ctx.Field("name").Index(0)` pattern +- **Clean composition**: Validators focus on validation, not path building +- **Reusable validators**: Same validator can be called with different contexts +- **No global state**: Thread-safe validation with explicit context + +#### **Type Safety with Constrained Values** +Following Go best practices used throughout the project: +- **Typed string constants**: `ValidationIssueType`, `ValidationIssueSeverity` prevent invalid values +- **Compile-time validation**: IDE autocomplete and error checking +- **JSON compatibility**: Still serializes as strings for API compatibility +- **Refactoring safety**: Rename constants without breaking code +- **Consistent with project**: Matches patterns used in `Status`, `Format`, `ArgumentType` + +### Performance Considerations +- **Slightly slower than fail-fast validation**: Acceptable trade-off for better user experience +- **Memory usage increases with error collection**: Manageable for typical server.json files +- **Schema validation performance**: Embedded schema eliminates I/O operations + +### Testing Strategy +- **Unit tests**: Each validator with context +- **Integration tests**: End-to-end validation testing +- **Backward compatibility tests**: Ensure existing code continues to work +- **Performance benchmarks**: Validate acceptable performance characteristics + +--- + +## Appendix: Future Enhancements + +### Additional Validation Types +- **Linter rules**: Best practices and style guidelines +- **Warning level**: Non-critical issues +- **Info level**: Suggestions and improvements + +### Advanced Features +- **Error filtering**: By type, severity, path pattern +- **Output formatting**: Human-readable, JSON, XML +- **Configuration**: Custom validation rules +- **IDE integration**: Real-time validation feedback + +### Tooling Integration +- **WASM package**: Browser-based validation +- **VS Code extension**: Real-time validation +- **CI/CD integration**: Automated validation in pipelines +- **API endpoint**: Validation as a service + + + + diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index ff1d9c90..49cf0bbb 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -122,6 +122,45 @@ mcp-publisher login none [--registry=URL] - No authentication - for local testing only - Only works with local registry instances +### `mcp-publisher validate` + +Validate a `server.json` file without publishing. + +**Usage:** +```bash +mcp-publisher validate [file] +``` + +**Arguments:** +- `file` - Path to server.json file (default: `./server.json`) + +**Behavior:** +- Performs exhaustive validation, reporting all issues at once (not just the first error) +- Validates JSON syntax and schema compliance +- Runs semantic validation (business logic checks) +- Checks for deprecated schema versions and provides migration guidance +- Includes detailed error locations with JSON paths (e.g., `packages[0].transport.url`) +- Shows validation issue type (json, schema, semantic, linter) +- Displays severity level (error, warning, info) +- Provides schema references showing which validation rule triggered each error + +**Example output:** +```bash +$ mcp-publisher validate +✅ server.json is valid + +$ mcp-publisher validate custom-server.json +❌ Validation failed with 2 issue(s): + +1. [error] repository.url (schema) + '' has invalid format 'uri' + Reference: #/definitions/Repository/properties/url/format from: [#/definitions/ServerDetail]/properties/repository/[#/definitions/Repository]/properties/url/format + +2. [error] name (semantic) + server name must be in format 'dns-namespace/name' + Reference: invalid-server-name +``` + ### `mcp-publisher publish` Publish server to the registry. diff --git a/internal/validators/schema.go b/internal/validators/schema.go new file mode 100644 index 00000000..1c366157 --- /dev/null +++ b/internal/validators/schema.go @@ -0,0 +1,307 @@ +package validators + +import ( + "bytes" + _ "embed" + "encoding/json" + "errors" + "fmt" + "strconv" + "strings" + + apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0" + "github.com/santhosh-tekuri/jsonschema/v5" +) + +//go:embed schema/server.schema.json +var embeddedSchema []byte + +// GetCurrentSchemaVersion extracts the $id field from the embedded schema +func GetCurrentSchemaVersion() (string, error) { + var schema map[string]any + if err := json.Unmarshal(embeddedSchema, &schema); err != nil { + return "", fmt.Errorf("failed to parse embedded schema: %w", err) + } + + id, ok := schema["$id"].(string) + if !ok { + return "", fmt.Errorf("embedded schema missing $id field") + } + + return id, nil +} + +// validateServerJSONSchema validates the server JSON against server.schema.json using jsonschema +func validateServerJSONSchema(serverJSON *apiv0.ServerJSON) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + + // Use embedded schema - no file system access needed + schemaData := embeddedSchema + + // Parse the schema + var schema map[string]any + if err := json.Unmarshal(schemaData, &schema); err != nil { + // If we can't parse the schema, return an error + issue := NewValidationIssue( + ValidationIssueTypeSchema, + "", + fmt.Sprintf("failed to parse schema file: %v", err), + ValidationIssueSeverityError, + "schema-parse-error", + ) + result.AddIssue(issue) + return result + } + + // Convert the server JSON to a map for validation + serverData, err := json.Marshal(serverJSON) + if err != nil { + issue := NewValidationIssue( + ValidationIssueTypeJSON, + "", + fmt.Sprintf("failed to marshal server JSON for schema validation: %v", err), + ValidationIssueSeverityError, + "json-marshal-error", + ) + result.AddIssue(issue) + return result + } + + var serverMap map[string]any + if err := json.Unmarshal(serverData, &serverMap); err != nil { + issue := NewValidationIssue( + ValidationIssueTypeJSON, + "", + fmt.Sprintf("failed to unmarshal server JSON for schema validation: %v", err), + ValidationIssueSeverityError, + "json-unmarshal-error", + ) + result.AddIssue(issue) + return result + } + + // Validate against schema using jsonschema library + compiler := jsonschema.NewCompiler() + if err := compiler.AddResource("file:///server.schema.json", bytes.NewReader(schemaData)); err != nil { + // If we can't add the schema resource, return an error + issue := NewValidationIssue( + ValidationIssueTypeSchema, + "", + fmt.Sprintf("failed to add schema resource: %v", err), + ValidationIssueSeverityError, + "schema-resource-error", + ) + result.AddIssue(issue) + return result + } + + schemaInstance, err := compiler.Compile("file:///server.schema.json") + if err != nil { + // If we can't compile the schema, return an error + issue := NewValidationIssue( + ValidationIssueTypeSchema, + "", + fmt.Sprintf("failed to compile schema: %v", err), + ValidationIssueSeverityError, + "schema-compile-error", + ) + result.AddIssue(issue) + return result + } + + // Perform validation + if err := schemaInstance.Validate(serverMap); err != nil { + // Convert validation error to our issue format + var validationErr *jsonschema.ValidationError + if errors.As(err, &validationErr) { + // Process the validation error and its causes + addValidationError(result, validationErr, schema) + } else { + // Fallback for other error types + issue := NewValidationIssue( + ValidationIssueTypeSchema, + "", + fmt.Sprintf("schema validation failed: %v", err), + ValidationIssueSeverityError, + "schema-validation-error", + ) + result.AddIssue(issue) + } + } + + return result +} + +// addValidationError processes validation errors and extracts useful information +func addValidationError(result *ValidationResult, validationErr *jsonschema.ValidationError, schema map[string]any) { + // Use DetailedOutput to get the nested error details + detailed := validationErr.DetailedOutput() + + // Process the detailed error structure + + addDetailedErrors(result, detailed, schema) +} + +// addDetailedErrors recursively processes detailed validation errors +func addDetailedErrors(result *ValidationResult, detailed jsonschema.Detailed, schema map[string]any) { + // Only process errors that have specific field paths and meaningful messages + if detailed.InstanceLocation != "" && detailed.Error != "" { + // Convert JSON Pointer to readable path (remove leading slash, convert / to .) + path := strings.TrimPrefix(detailed.InstanceLocation, "/") + path = strings.ReplaceAll(path, "/", ".") + + // Clean up the error message + message := detailed.Error + + // Make messages more user-friendly + if strings.Contains(message, "missing properties:") { + message = strings.ReplaceAll(message, "missing properties:", "missing required fields:") + } + if strings.Contains(message, "is not valid") { + message = strings.ReplaceAll(message, "is not valid", "has invalid format") + } + + // Build the full resolved reference path + reference := buildResolvedReference(detailed.KeywordLocation, detailed.AbsoluteKeywordLocation, schema) + + issue := NewValidationIssue( + ValidationIssueTypeSchema, + path, + message, + ValidationIssueSeverityError, + reference, // cleaned schema rule path for deterministic mapping + ) + result.AddIssue(issue) + } + + // Process nested errors + for _, nested := range detailed.Errors { + addDetailedErrors(result, nested, schema) + } +} + +// buildResolvedReference extracts the resolved reference path by resolving $ref segments +func buildResolvedReference(keywordLocation, absoluteKeywordLocation string, schema map[string]any) string { + if keywordLocation == "" || absoluteKeywordLocation == "" { + return "" + } + + // Clean up the absolute location by removing file:// prefix + absolute := absoluteKeywordLocation + if strings.HasPrefix(absolute, "file://") { + absolute = strings.TrimPrefix(absolute, "file://") + if idx := strings.Index(absolute, "#"); idx != -1 { + absolute = absolute[idx:] // Keep only the #/path part + } + } + + // Parse the keyword location to understand the $ref chain + keyword := strings.TrimPrefix(keywordLocation, "/") + keywordParts := strings.Split(keyword, "/") + + // Build the path showing $ref resolution + pathSegments := make([]string, 0) + + // Track the resolved path so far (starts empty, gets built up as we resolve $refs) + resolvedPath := "" + + // Process each part of the keyword path + for i, part := range keywordParts { + if part == "" { + continue // Skip empty parts + } + + if part == "$ref" { + // This is a $ref - we need to look up what it resolves to + // For the first $ref, use the path from the root + // For subsequent $refs, use the resolved path from the previous $ref plus the current segment + var refPath string + if resolvedPath == "" { + // First $ref - use the path from the root + refPath = strings.Join(keywordParts[:i+1], "/") + refPath = "/" + refPath + } else { + // Subsequent $ref - use the resolved path plus the current segment + refPath = resolvedPath + "/" + part + } + + // Look up the $ref value in the schema + refValue := resolveRefInSchema(schema, refPath) + + if refValue != "" { + pathSegments = append(pathSegments, fmt.Sprintf("[%s]", refValue)) + // Update the resolved path for the next $ref + resolvedPath = refValue + } else { + pathSegments = append(pathSegments, "[$ref]") + } + } else { + // Regular path segment + pathSegments = append(pathSegments, part) + // Add this segment to the resolved path for the next $ref + if resolvedPath != "" { + resolvedPath = resolvedPath + "/" + part + } else { + resolvedPath = part + } + } + } + + // Build the final reference string + if len(pathSegments) > 0 { + pathStr := strings.Join(pathSegments, "/") + return fmt.Sprintf("%s from: %s", absolute, pathStr) + } + + // Fallback: return the absolute location with context + return absolute + " (from: " + keywordLocation + ")" +} + +// resolveRefInSchema looks up a $ref value in the schema +func resolveRefInSchema(schema map[string]any, refPath string) string { + // Handle the # prefix - it indicates the root of the schema JSON + refPath = strings.TrimPrefix(refPath, "#") + + // Parse the JSON pointer path + pathParts := strings.Split(strings.TrimPrefix(refPath, "/"), "/") + + // Navigate through the schema to find the $ref value + var current any = schema + for _, part := range pathParts { + if part == "" { + continue + } + + if part == "$ref" { + // We've reached the $ref, return its value + if currentMap, ok := current.(map[string]any); ok { + if refValue, ok := currentMap["$ref"].(string); ok { + return refValue + } + } + return "" + } + + // Navigate to the next level + // Check if this is an array index + if index, err := strconv.Atoi(part); err == nil { + // This is an array index - check if current element is an array + if arr, ok := current.([]any); ok && index < len(arr) { + current = arr[index] + } else { + // Current element is not an array or index out of bounds + return "" + } + } else { + // This is a map key + if currentMap, ok := current.(map[string]any); ok { + current = currentMap[part] + } else { + // Current element is not a map + return "" + } + } + } + + return "" +} diff --git a/internal/validators/validation_detailed_test.go b/internal/validators/validation_detailed_test.go new file mode 100644 index 00000000..cff13cfc --- /dev/null +++ b/internal/validators/validation_detailed_test.go @@ -0,0 +1,258 @@ +package validators_test + +import ( + "testing" + + "github.com/modelcontextprotocol/registry/internal/validators" + apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0" + "github.com/modelcontextprotocol/registry/pkg/model" + "github.com/stretchr/testify/assert" +) + +func TestValidateServerJSONExhaustive_CollectsAllErrors(t *testing.T) { + // Create a server JSON with multiple validation errors + serverJSON := &apiv0.ServerJSON{ + Name: "invalid-name", // Invalid server name format + Version: "^1.0.0", // Invalid version range + Description: "Test server", + Repository: model.Repository{ + URL: "not-a-valid-url", // Invalid repository URL + Source: "github", + }, + WebsiteURL: "ftp://invalid-scheme.com", // Invalid website URL scheme + Packages: []model.Package{ + { + RegistryType: model.RegistryTypeOCI, + RegistryBaseURL: "https://docker.io", + Identifier: "package with spaces", // Invalid package name + Version: "latest", // Reserved version + Transport: model.Transport{ + Type: model.TransportTypeStdio, + URL: "should-not-have-url", // Invalid stdio transport with URL + }, + RuntimeArguments: []model.Argument{ + { + Type: model.ArgumentTypeNamed, + Name: "--port ", // Invalid argument name + }, + }, + }, + }, + Remotes: []model.Transport{ + { + Type: model.TransportTypeStdio, // Invalid remote transport type + URL: "", // Missing URL for remote + }, + }, + } + + // Run detailed validation + result := validators.ValidateServerJSONExhaustive(serverJSON, false) + + // Verify it's invalid + assert.False(t, result.Valid) + assert.Greater(t, len(result.Issues), 5, "Should have multiple validation issues") + + // Check that we have issues of different types and severities + hasError := false + hasSemantic := false + + for _, issue := range result.Issues { + if issue.Severity == validators.ValidationIssueSeverityError { + hasError = true + } + if issue.Type == validators.ValidationIssueTypeSemantic { + hasSemantic = true + } + } + + assert.True(t, hasError, "Should have error severity issues") + assert.True(t, hasSemantic, "Should have semantic type issues") + + // Verify specific issues exist + issuePaths := make(map[string]bool) + for _, issue := range result.Issues { + issuePaths[issue.Path] = true + } + + // Check for expected issue paths + expectedPaths := []string{ + "name", + "version", + "repository.url", + "websiteUrl", + "packages[0].identifier", + "packages[0].version", + "packages[0].transport.url", + "packages[0].runtimeArguments[0].name", + "remotes[0].type", + "remotes[0].url", + } + + foundPaths := 0 + for _, expectedPath := range expectedPaths { + if issuePaths[expectedPath] { + foundPaths++ + } + } + + assert.Greater(t, foundPaths, 5, "Should have issues at multiple JSON paths") +} + +func TestValidateServerJSONExhaustive_ValidServer(t *testing.T) { + // Create a valid server JSON + serverJSON := &apiv0.ServerJSON{ + Schema: model.CurrentSchemaURL, + Name: "com.example.test/valid-server", + Version: "1.0.0", + Description: "A valid test server", + Repository: model.Repository{ + URL: "https://github.com/example/valid-server", + Source: "github", + }, + WebsiteURL: "https://test.example.com", + Packages: []model.Package{ + { + RegistryType: model.RegistryTypeOCI, + RegistryBaseURL: "https://docker.io", + Identifier: "valid-package", + Version: "1.0.0", + Transport: model.Transport{ + Type: model.TransportTypeStdio, + }, + }, + }, + } + + // Run detailed validation + result := validators.ValidateServerJSONExhaustive(serverJSON, false) + + // Verify it's valid + assert.True(t, result.Valid) + assert.Empty(t, result.Issues, "Should have no validation issues") +} + +func TestValidateServerJSONExhaustive_ContextPaths(t *testing.T) { + // Create a server with nested validation errors to test context paths + serverJSON := &apiv0.ServerJSON{ + Name: "com.example.test/server", + Version: "1.0.0", + Packages: []model.Package{ + { + RegistryType: model.RegistryTypeOCI, + RegistryBaseURL: "https://docker.io", + Identifier: "package-1", + Version: "latest", // Error in first package + Transport: model.Transport{ + Type: model.TransportTypeStdio, + }, + }, + { + RegistryType: model.RegistryTypeOCI, + RegistryBaseURL: "https://docker.io", + Identifier: "package-2", + Version: "2.0.0", + Transport: model.Transport{ + Type: model.TransportTypeStdio, + }, + RuntimeArguments: []model.Argument{ + { + Type: model.ArgumentTypeNamed, + Name: "invalid name", // Error in second package's argument + }, + }, + }, + }, + } + + // Run detailed validation + result := validators.ValidateServerJSONExhaustive(serverJSON, false) + + // Verify we have issues at the correct paths + issuePaths := make(map[string]bool) + for _, issue := range result.Issues { + issuePaths[issue.Path] = true + } + + // Should have issues at specific nested paths + assert.True(t, issuePaths["packages[0].version"], "Should have issue at packages[0].version") + assert.True(t, issuePaths["packages[1].runtimeArguments[0].name"], "Should have issue at packages[1].runtimeArguments[0].name") +} + +func TestValidateServerJSONExhaustive_RefResolution(t *testing.T) { + // Create a server JSON with validation errors that will trigger $ref resolution + serverJSON := &apiv0.ServerJSON{ + Schema: model.CurrentSchemaURL, + Name: "com.example.test/invalid-server", + Version: "1.0.0", + Description: "Test server with validation errors", + Repository: model.Repository{ + URL: "", // Empty URL should trigger format validation error in $ref'd Repository + Source: "github", + }, + Packages: []model.Package{ + { + RegistryType: model.RegistryTypeOCI, + RegistryBaseURL: "https://docker.io", + Identifier: "test-package", + Version: "1.0.0", + Transport: model.Transport{ + Type: model.TransportTypeSSE, + URL: "https://example.com", + }, + PackageArguments: []model.Argument{ + { + InputWithVariables: model.InputWithVariables{ + Input: model.Input{ + Format: "invalid-format", // This should trigger a validation error in the complex path + }, + }, + Type: "named", + Name: "test-arg", + }, + }, + }, + }, + } + + // Run validation with schema validation enabled + result := validators.ValidateServerJSONExhaustive(serverJSON, true) + + // Check that we have validation errors + assert.False(t, result.Valid, "Expected validation errors") + assert.Greater(t, len(result.Issues), 0, "Expected at least one validation issue") + + // Check that we have schema validation issues with proper $ref resolution + hasSchemaIssues := false + for _, issue := range result.Issues { + if issue.Type == validators.ValidationIssueTypeSchema { + hasSchemaIssues = true + // Check that there are no unresolved [$ref] segments + assert.NotContains(t, issue.Reference, "[$ref]", "Found unresolved $ref segment in reference: %s", issue.Reference) + + // Check for exact resolved paths we expect + if issue.Path == "repository.url" { + expectedRef := "#/definitions/Repository/properties/url/format from: [#/definitions/ServerDetail]/properties/repository/[#/definitions/Repository]/properties/url/format" + assert.Equal(t, expectedRef, issue.Reference, "Repository URL error should have exact resolved reference") + } + if issue.Path == "packages.0.packageArguments.0.format" { + // The schema uses anyOf for Argument types, so it could match either PositionalArgument or NamedArgument + // Just check that it contains the expected definitions + assert.Contains(t, issue.Reference, "#/definitions/Input/properties/format/enum", "Should reference the Input format enum") + assert.Contains(t, issue.Reference, "[#/definitions/InputWithVariables]", "Should reference InputWithVariables") + assert.Contains(t, issue.Reference, "[#/definitions/Input]", "Should reference Input") + } + } + } + assert.True(t, hasSchemaIssues, "Expected schema validation issues with $ref resolution") + + // Check that we have issues at expected paths + issuePaths := make(map[string]bool) + for _, issue := range result.Issues { + issuePaths[issue.Path] = true + } + + // Should have issues at specific paths that trigger $ref resolution + assert.True(t, issuePaths["repository.url"], "Should have issue at repository.url") + assert.True(t, issuePaths["packages.0.packageArguments.0.format"], "Should have issue at packages.0.packageArguments.0.format") +} diff --git a/internal/validators/validation_types.go b/internal/validators/validation_types.go new file mode 100644 index 00000000..dfd1a418 --- /dev/null +++ b/internal/validators/validation_types.go @@ -0,0 +1,98 @@ +package validators + +import "fmt" + +// Validation issue type with constrained values +type ValidationIssueType string + +const ( + ValidationIssueTypeJSON ValidationIssueType = "json" + ValidationIssueTypeSchema ValidationIssueType = "schema" + ValidationIssueTypeSemantic ValidationIssueType = "semantic" + ValidationIssueTypeLinter ValidationIssueType = "linter" +) + +// Validation issue severity with constrained values +type ValidationIssueSeverity string + +const ( + ValidationIssueSeverityError ValidationIssueSeverity = "error" + ValidationIssueSeverityWarning ValidationIssueSeverity = "warning" + ValidationIssueSeverityInfo ValidationIssueSeverity = "info" +) + +// ValidationIssue represents a single validation problem +type ValidationIssue struct { + Type ValidationIssueType `json:"type"` + Path string `json:"path"` // JSON path like "packages[0].transport.url" + Message string `json:"message"` // Error description (extracted from error.Error()) + Severity ValidationIssueSeverity `json:"severity"` + Reference string `json:"reference"` // Reference to validation trigger (schema rule path, named rule, etc.) +} + +// ValidationResult contains the results of validation +type ValidationResult struct { + Valid bool `json:"valid"` + Issues []ValidationIssue `json:"issues"` +} + +// ValidationContext tracks the current JSON path during validation +type ValidationContext struct { + path string +} + +// NewValidationIssue creates a validation issue with manual field setting +func NewValidationIssue(issueType ValidationIssueType, path, message string, severity ValidationIssueSeverity, reference string) ValidationIssue { + return ValidationIssue{ + Type: issueType, + Path: path, + Message: message, + Severity: severity, + Reference: reference, + } +} + +// NewValidationIssueFromError creates a validation issue from an existing error +func NewValidationIssueFromError(issueType ValidationIssueType, path string, err error, reference string) ValidationIssue { + return ValidationIssue{ + Type: issueType, + Path: path, + Message: err.Error(), // Extract string from error + Severity: ValidationIssueSeverityError, // Errors are always severity "error" + Reference: reference, + } +} + +// AddIssue adds a validation issue to the result +func (vr *ValidationResult) AddIssue(issue ValidationIssue) { + vr.Issues = append(vr.Issues, issue) + if issue.Severity == ValidationIssueSeverityError { + vr.Valid = false + } +} + +// Merge combines another validation result into this one +func (vr *ValidationResult) Merge(other *ValidationResult) { + vr.Issues = append(vr.Issues, other.Issues...) + if !other.Valid { + vr.Valid = false + } +} + +// Field adds a field name to the context path +func (ctx *ValidationContext) Field(name string) *ValidationContext { + if ctx.path == "" { + return &ValidationContext{path: name} + } + return &ValidationContext{path: ctx.path + "." + name} +} + +// Index adds an array index to the context path +func (ctx *ValidationContext) Index(i int) *ValidationContext { + return &ValidationContext{path: ctx.path + fmt.Sprintf("[%d]", i)} +} + +// String returns the current path as a string +func (ctx *ValidationContext) String() string { + return ctx.path +} diff --git a/internal/validators/validation_types_test.go b/internal/validators/validation_types_test.go new file mode 100644 index 00000000..8d8a0841 --- /dev/null +++ b/internal/validators/validation_types_test.go @@ -0,0 +1,173 @@ +package validators_test + +import ( + "errors" + "testing" + + "github.com/modelcontextprotocol/registry/internal/validators" + "github.com/stretchr/testify/assert" +) + +func TestValidationIssueTypes(t *testing.T) { + tests := []struct { + name string + issueType validators.ValidationIssueType + expected string + }{ + {"JSON type", validators.ValidationIssueTypeJSON, "json"}, + {"Schema type", validators.ValidationIssueTypeSchema, "schema"}, + {"Semantic type", validators.ValidationIssueTypeSemantic, "semantic"}, + {"Linter type", validators.ValidationIssueTypeLinter, "linter"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, string(tt.issueType)) + }) + } +} + +func TestValidationIssueSeverity(t *testing.T) { + tests := []struct { + name string + severity validators.ValidationIssueSeverity + expected string + }{ + {"Error severity", validators.ValidationIssueSeverityError, "error"}, + {"Warning severity", validators.ValidationIssueSeverityWarning, "warning"}, + {"Info severity", validators.ValidationIssueSeverityInfo, "info"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, string(tt.severity)) + }) + } +} + +func TestNewValidationIssue(t *testing.T) { + issue := validators.NewValidationIssue( + validators.ValidationIssueTypeSemantic, + "repository.url", + "invalid repository URL", + validators.ValidationIssueSeverityError, + "invalid-repository-url", + ) + + assert.Equal(t, validators.ValidationIssueTypeSemantic, issue.Type) + assert.Equal(t, "repository.url", issue.Path) + assert.Equal(t, "invalid repository URL", issue.Message) + assert.Equal(t, validators.ValidationIssueSeverityError, issue.Severity) + assert.Equal(t, "invalid-repository-url", issue.Reference) +} + +func TestNewValidationIssueFromError(t *testing.T) { + err := errors.New("invalid repository URL: https://bad-url.com") + issue := validators.NewValidationIssueFromError( + validators.ValidationIssueTypeSemantic, + "repository.url", + err, + "invalid-repository-url", + ) + + assert.Equal(t, validators.ValidationIssueTypeSemantic, issue.Type) + assert.Equal(t, "repository.url", issue.Path) + assert.Equal(t, "invalid repository URL: https://bad-url.com", issue.Message) + assert.Equal(t, validators.ValidationIssueSeverityError, issue.Severity) + assert.Equal(t, "invalid-repository-url", issue.Reference) +} + +func TestValidationResultAddIssue(t *testing.T) { + result := &validators.ValidationResult{Valid: true, Issues: []validators.ValidationIssue{}} + + // Add a warning issue - should not affect validity + warningIssue := validators.NewValidationIssue( + validators.ValidationIssueTypeLinter, + "description", + "consider adding a description", + validators.ValidationIssueSeverityWarning, + "descriptive-naming", + ) + result.AddIssue(warningIssue) + + assert.True(t, result.Valid) + assert.Len(t, result.Issues, 1) + + // Add an error issue - should make invalid + errorIssue := validators.NewValidationIssue( + validators.ValidationIssueTypeSemantic, + "name", + "server name is required", + validators.ValidationIssueSeverityError, + "missing-server-name", + ) + result.AddIssue(errorIssue) + + assert.False(t, result.Valid) + assert.Len(t, result.Issues, 2) +} + +func TestValidationResultMerge(t *testing.T) { + result1 := &validators.ValidationResult{Valid: true, Issues: []validators.ValidationIssue{}} + result2 := &validators.ValidationResult{Valid: false, Issues: []validators.ValidationIssue{}} + + // Add issues to both + issue1 := validators.NewValidationIssue( + validators.ValidationIssueTypeSemantic, + "name", + "server name is required", + validators.ValidationIssueSeverityError, + "missing-server-name", + ) + result1.AddIssue(issue1) + + issue2 := validators.NewValidationIssue( + validators.ValidationIssueTypeSchema, + "version", + "version must be a string", + validators.ValidationIssueSeverityError, + "schema-validation", + ) + result2.AddIssue(issue2) + + // Merge result2 into result1 + result1.Merge(result2) + + assert.False(t, result1.Valid) // Should be invalid because result2 was invalid + assert.Len(t, result1.Issues, 2) // Should have both issues +} + +func TestValidationContext(t *testing.T) { + // Test empty context + ctx := &validators.ValidationContext{} + assert.Equal(t, "", ctx.String()) + + // Test field addition + ctx = ctx.Field("repository") + assert.Equal(t, "repository", ctx.String()) + + // Test nested field + ctx = ctx.Field("url") + assert.Equal(t, "repository.url", ctx.String()) + + // Test array index + ctx = &validators.ValidationContext{} + ctx = ctx.Field("packages").Index(0).Field("transport") + assert.Equal(t, "packages[0].transport", ctx.String()) + + // Test multiple array indices + ctx = &validators.ValidationContext{} + ctx = ctx.Field("packages").Index(0).Field("environmentVariables").Index(1).Field("name") + assert.Equal(t, "packages[0].environmentVariables[1].name", ctx.String()) +} + +func TestValidationContextImmutability(t *testing.T) { + // Test that context operations return new instances + ctx1 := &validators.ValidationContext{} + ctx2 := ctx1.Field("repository") + ctx3 := ctx2.Field("url") + + assert.Equal(t, "", ctx1.String()) + assert.Equal(t, "repository", ctx2.String()) + assert.Equal(t, "repository.url", ctx3.String()) +} diff --git a/internal/validators/validators.go b/internal/validators/validators.go index dc70ff1f..29bba264 100644 --- a/internal/validators/validators.go +++ b/internal/validators/validators.go @@ -53,215 +53,333 @@ var ( ) func ValidateServerJSON(serverJSON *apiv0.ServerJSON) error { + result := ValidateServerJSONExhaustive(serverJSON, false) + if !result.Valid { + // Return the first error issue + for _, issue := range result.Issues { + if issue.Severity == ValidationIssueSeverityError { + return fmt.Errorf("%s", issue.Message) + } + } + } + return nil +} + +// ValidateServerJSONExhaustive performs exhaustive validation and returns all issues found +// If validateSchema is true, it will also validate against server.schema.json +func ValidateServerJSONExhaustive(serverJSON *apiv0.ServerJSON, validateSchema bool) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + ctx := &ValidationContext{} + // Validate schema version is provided and supported // Note: Schema field is also marked as required in the ServerJSON struct definition // for API-level validation and documentation - if serverJSON.Schema == "" { - return fmt.Errorf("$schema field is required") - } - if !strings.Contains(serverJSON.Schema, model.CurrentSchemaVersion) { - return fmt.Errorf("schema version %s is not supported. Please use schema version %s", serverJSON.Schema, model.CurrentSchemaVersion) + switch { + case serverJSON.Schema == "": + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("schema").String(), + fmt.Errorf("$schema field is required"), + "schema-field-required", + ) + result.AddIssue(issue) + case !strings.Contains(serverJSON.Schema, model.CurrentSchemaVersion): + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("schema").String(), + fmt.Errorf("schema version %s is not supported. Please use schema version %s", serverJSON.Schema, model.CurrentSchemaVersion), + "schema-version-not-supported", + ) + result.AddIssue(issue) + case validateSchema: + // We have a valid schema version and validation requested + schemaResult := validateServerJSONSchema(serverJSON) + result.Merge(schemaResult) } // Validate server name exists and format if _, err := parseServerName(*serverJSON); err != nil { - return err + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("name").String(), + err, + "invalid-server-name", + ) + result.AddIssue(issue) } // Validate top-level server version is a specific version (not a range) & not "latest" - if err := validateVersion(serverJSON.Version); err != nil { - return err - } + versionResult := validateVersion(ctx.Field("version"), serverJSON.Version) + result.Merge(versionResult) // Validate repository - if err := validateRepository(&serverJSON.Repository); err != nil { - return err - } + repoResult := validateRepository(ctx.Field("repository"), &serverJSON.Repository) + result.Merge(repoResult) // Validate website URL if provided - if err := validateWebsiteURL(serverJSON.WebsiteURL); err != nil { - return err - } + websiteResult := validateWebsiteURL(ctx.Field("websiteUrl"), serverJSON.WebsiteURL) + result.Merge(websiteResult) // Validate title if provided - if err := validateTitle(serverJSON.Title); err != nil { - return err - } + titleResult := validateTitle(ctx.Field("title"), serverJSON.Title) + result.Merge(titleResult) // Validate icons if provided - if err := validateIcons(serverJSON.Icons); err != nil { - return err - } + iconsResult := validateIcons(ctx.Field("icons"), serverJSON.Icons) + result.Merge(iconsResult) // Validate all packages (basic field validation) // Detailed package validation (including registry checks) is done during publish - for _, pkg := range serverJSON.Packages { - if err := validatePackageField(&pkg); err != nil { - return err - } + for i, pkg := range serverJSON.Packages { + pkgResult := validatePackageField(ctx.Field("packages").Index(i), &pkg) + result.Merge(pkgResult) } // Validate all remotes - for _, remote := range serverJSON.Remotes { - if err := validateRemoteTransport(&remote); err != nil { - return err - } + for i, remote := range serverJSON.Remotes { + remoteResult := validateRemoteTransport(ctx.Field("remotes").Index(i), &remote) + result.Merge(remoteResult) } // Validate reverse-DNS namespace matching for remote URLs - if err := validateRemoteNamespaceMatch(*serverJSON); err != nil { - return err - } + remoteNamespaceResult := validateRemoteNamespaceMatch(ctx.Field("remotes"), *serverJSON) + result.Merge(remoteNamespaceResult) // Validate reverse-DNS namespace matching for website URL - if err := validateWebsiteURLNamespaceMatch(*serverJSON); err != nil { - return err - } + websiteNamespaceResult := validateWebsiteURLNamespaceMatch(ctx.Field("websiteUrl"), *serverJSON) + result.Merge(websiteNamespaceResult) - return nil + return result } -func validateRepository(obj *model.Repository) error { +func validateRepository(ctx *ValidationContext, obj *model.Repository) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Skip validation for empty repository (optional field) if obj.URL == "" && obj.Source == "" { - return nil + return result } // validate the repository source repoSource := RepositorySource(obj.Source) if !IsValidRepositoryURL(repoSource, obj.URL) { - return fmt.Errorf("%w: %s", ErrInvalidRepositoryURL, obj.URL) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("url").String(), + fmt.Errorf("%w: %s", ErrInvalidRepositoryURL, obj.URL), + "invalid-repository-url", + ) + result.AddIssue(issue) } // validate subfolder if present if obj.Subfolder != "" && !IsValidSubfolderPath(obj.Subfolder) { - return fmt.Errorf("%w: %s", ErrInvalidSubfolderPath, obj.Subfolder) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("subfolder").String(), + fmt.Errorf("%w: %s", ErrInvalidSubfolderPath, obj.Subfolder), + "invalid-subfolder-path", + ) + result.AddIssue(issue) } - return nil + return result } -func validateWebsiteURL(websiteURL string) error { +func validateWebsiteURL(ctx *ValidationContext, websiteURL string) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Skip validation if website URL is not provided (optional field) if websiteURL == "" { - return nil + return result } // Parse the URL to ensure it's valid parsedURL, err := url.Parse(websiteURL) if err != nil { - return fmt.Errorf("invalid websiteUrl: %w", err) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.String(), + fmt.Errorf("invalid websiteUrl: %w", err), + "invalid-website-url", + ) + result.AddIssue(issue) + return result } // Ensure it's an absolute URL with valid scheme if !parsedURL.IsAbs() { - return fmt.Errorf("websiteUrl must be absolute (include scheme): %s", websiteURL) + issue := NewValidationIssue( + ValidationIssueTypeSemantic, + ctx.String(), + fmt.Sprintf("websiteUrl must be absolute (include scheme): %s", websiteURL), + ValidationIssueSeverityError, + "website-url-must-be-absolute", + ) + result.AddIssue(issue) } // Only allow HTTPS scheme for security if parsedURL.Scheme != SchemeHTTPS { - return fmt.Errorf("websiteUrl must use https scheme: %s", websiteURL) + issue := NewValidationIssue( + ValidationIssueTypeSemantic, + ctx.String(), + fmt.Sprintf("websiteUrl must use https scheme: %s", websiteURL), + ValidationIssueSeverityError, + "website-url-invalid-scheme", + ) + result.AddIssue(issue) } - return nil + return result } -func validateTitle(title string) error { +func validateTitle(ctx *ValidationContext, title string) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Skip validation if title is not provided (optional field) if title == "" { - return nil + return result } // Check that title is not only whitespace if strings.TrimSpace(title) == "" { - return fmt.Errorf("title cannot be only whitespace") + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.String(), + fmt.Errorf("title cannot be only whitespace"), + "title-whitespace-only", + ) + result.AddIssue(issue) } - return nil + return result } -func validateIcons(icons []model.Icon) error { +func validateIcons(ctx *ValidationContext, icons []model.Icon) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Skip validation if no icons are provided (optional field) if len(icons) == 0 { - return nil + return result } // Validate each icon for i, icon := range icons { - if err := validateIcon(&icon); err != nil { - return fmt.Errorf("invalid icon at index %d: %w", i, err) - } + iconResult := validateIcon(ctx.Index(i), &icon) + result.Merge(iconResult) } - return nil + return result } -func validateIcon(icon *model.Icon) error { +func validateIcon(ctx *ValidationContext, icon *model.Icon) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Parse the URL to ensure it's valid parsedURL, err := url.Parse(icon.Src) if err != nil { - return fmt.Errorf("invalid icon src URL: %w", err) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("src").String(), + fmt.Errorf("invalid icon src URL: %w", err), + "icon-src-invalid-url", + ) + result.AddIssue(issue) + return result } // Ensure it's an absolute URL if !parsedURL.IsAbs() { - return fmt.Errorf("icon src must be an absolute URL (include scheme): %s", icon.Src) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("src").String(), + fmt.Errorf("icon src must be an absolute URL (include scheme): %s", icon.Src), + "icon-src-not-absolute", + ) + result.AddIssue(issue) } // Only allow HTTPS scheme for security (no HTTP or data: URIs) if parsedURL.Scheme != SchemeHTTPS { - return fmt.Errorf("icon src must use https scheme (got %s): %s", parsedURL.Scheme, icon.Src) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("src").String(), + fmt.Errorf("icon src must use https scheme (got %s): %s", parsedURL.Scheme, icon.Src), + "icon-src-invalid-scheme", + ) + result.AddIssue(issue) } - return nil + return result } -func validatePackageField(obj *model.Package) error { +func validatePackageField(ctx *ValidationContext, obj *model.Package) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + + // Validate identifier has no spaces if !HasNoSpaces(obj.Identifier) { - return ErrPackageNameHasSpaces + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("identifier").String(), + ErrPackageNameHasSpaces, + "package-name-has-spaces", + ) + result.AddIssue(issue) } // Validate version string - if err := validateVersion(obj.Version); err != nil { - return err - } + versionResult := validateVersion(ctx.Field("version"), obj.Version) + result.Merge(versionResult) // Validate runtime arguments - for _, arg := range obj.RuntimeArguments { - if err := validateArgument(&arg); err != nil { - return fmt.Errorf("invalid runtime argument: %w", err) - } + for i, arg := range obj.RuntimeArguments { + argResult := validateArgument(ctx.Field("runtimeArguments").Index(i), &arg) + result.Merge(argResult) } // Validate package arguments - for _, arg := range obj.PackageArguments { - if err := validateArgument(&arg); err != nil { - return fmt.Errorf("invalid package argument: %w", err) - } + for i, arg := range obj.PackageArguments { + argResult := validateArgument(ctx.Field("packageArguments").Index(i), &arg) + result.Merge(argResult) } // Validate transport with template variable support availableVariables := collectAvailableVariables(obj) - if err := validatePackageTransport(&obj.Transport, availableVariables); err != nil { - return fmt.Errorf("invalid transport: %w", err) - } + transportResult := validatePackageTransport(ctx.Field("transport"), &obj.Transport, availableVariables) + result.Merge(transportResult) - return nil + return result } // validateVersion validates the version string. // NB: we decided that we would not enforce strict semver for version strings -func validateVersion(version string) error { +func validateVersion(ctx *ValidationContext, version string) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + if version == "latest" { - return ErrReservedVersionString + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.String(), + ErrReservedVersionString, + "reserved-version-string", + ) + result.AddIssue(issue) + return result } // Reject semver range-like inputs if looksLikeVersionRange(version) { - return fmt.Errorf("%w: %q", ErrVersionLooksLikeRange, version) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.String(), + fmt.Errorf("%w: %q", ErrVersionLooksLikeRange, version), + "version-looks-like-range", + ) + result.AddIssue(issue) } - return nil + return result } // looksLikeVersionRange detects common semver range syntaxes and wildcard patterns. @@ -297,25 +415,34 @@ func looksLikeVersionRange(version string) bool { } // validateArgument validates argument details -func validateArgument(obj *model.Argument) error { +func validateArgument(ctx *ValidationContext, obj *model.Argument) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + if obj.Type == model.ArgumentTypeNamed { // Validate named argument name format - if err := validateNamedArgumentName(obj.Name); err != nil { - return err - } + nameResult := validateNamedArgumentName(ctx.Field("name"), obj.Name) + result.Merge(nameResult) // Validate value and default don't start with the name - if err := validateArgumentValueFields(obj.Name, obj.Value, obj.Default); err != nil { - return err - } + valueResult := validateArgumentValueFields(ctx, obj.Name, obj.Value, obj.Default) + result.Merge(valueResult) } - return nil + return result } -func validateNamedArgumentName(name string) error { +func validateNamedArgumentName(ctx *ValidationContext, name string) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Check if name is required for named arguments if name == "" { - return ErrNamedArgumentNameRequired + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.String(), + ErrNamedArgumentNameRequired, + "named-argument-name-required", + ) + result.AddIssue(issue) + return result } // Check for invalid characters that suggest embedded values or descriptions @@ -323,23 +450,43 @@ func validateNamedArgumentName(name string) error { // Invalid: "--directory ", "--port 8080" if strings.Contains(name, "<") || strings.Contains(name, ">") || strings.Contains(name, " ") || strings.Contains(name, "$") { - return fmt.Errorf("%w: %s", ErrInvalidNamedArgumentName, name) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.String(), + fmt.Errorf("%w: %s", ErrInvalidNamedArgumentName, name), + "invalid-named-argument-name", + ) + result.AddIssue(issue) } - return nil + return result } -func validateArgumentValueFields(name, value, defaultValue string) error { +func validateArgumentValueFields(ctx *ValidationContext, name, value, defaultValue string) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Check if value starts with the argument name (using startsWith, not contains) if value != "" && strings.HasPrefix(value, name) { - return fmt.Errorf("%w: value starts with argument name '%s': %s", ErrArgumentValueStartsWithName, name, value) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("value").String(), + fmt.Errorf("%w: value starts with argument name '%s': %s", ErrArgumentValueStartsWithName, name, value), + "argument-value-starts-with-name", + ) + result.AddIssue(issue) } if defaultValue != "" && strings.HasPrefix(defaultValue, name) { - return fmt.Errorf("%w: default starts with argument name '%s': %s", ErrArgumentDefaultStartsWithName, name, defaultValue) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("default").String(), + fmt.Errorf("%w: default starts with argument name '%s': %s", ErrArgumentDefaultStartsWithName, name, defaultValue), + "argument-default-starts-with-name", + ) + result.AddIssue(issue) } - return nil + return result } // collectAvailableVariables collects all available template variables from a package @@ -375,53 +522,106 @@ func collectAvailableVariables(pkg *model.Package) []string { } // validatePackageTransport validates a package's transport with templating support -func validatePackageTransport(transport *model.Transport, availableVariables []string) error { +func validatePackageTransport(ctx *ValidationContext, transport *model.Transport, availableVariables []string) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Validate transport type is supported switch transport.Type { case model.TransportTypeStdio: // Validate that URL is empty for stdio transport if transport.URL != "" { - return fmt.Errorf("url must be empty for %s transport type, got: %s", transport.Type, transport.URL) + issue := NewValidationIssue( + ValidationIssueTypeSemantic, + ctx.Field("url").String(), + fmt.Sprintf("url must be empty for %s transport type, got: %s", transport.Type, transport.URL), + ValidationIssueSeverityError, + "stdio-transport-url-not-empty", + ) + result.AddIssue(issue) } - return nil case model.TransportTypeStreamableHTTP, model.TransportTypeSSE: // URL is required for streamable-http and sse if transport.URL == "" { - return fmt.Errorf("url is required for %s transport type", transport.Type) - } - // Validate URL format with template variable support - if !IsValidTemplatedURL(transport.URL, availableVariables, true) { + issue := NewValidationIssue( + ValidationIssueTypeSemantic, + ctx.Field("url").String(), + fmt.Sprintf("url is required for %s transport type", transport.Type), + ValidationIssueSeverityError, + "streamable-transport-url-required", + ) + result.AddIssue(issue) + } else if !IsValidTemplatedURL(transport.URL, availableVariables, true) { + // Validate URL format with template variable support // Check if it's a template variable issue or basic URL issue templateVars := extractTemplateVariables(transport.URL) + var err error if len(templateVars) > 0 { - return fmt.Errorf("%w: template variables in URL %s reference undefined variables. Available variables: %v", + err = fmt.Errorf("%w: template variables in URL %s reference undefined variables. Available variables: %v", ErrInvalidRemoteURL, transport.URL, availableVariables) + } else { + err = fmt.Errorf("%w: %s", ErrInvalidRemoteURL, transport.URL) } - return fmt.Errorf("%w: %s", ErrInvalidRemoteURL, transport.URL) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("url").String(), + err, + "invalid-templated-url", + ) + result.AddIssue(issue) } - return nil default: - return fmt.Errorf("unsupported transport type: %s", transport.Type) + issue := NewValidationIssue( + ValidationIssueTypeSemantic, + ctx.Field("type").String(), + fmt.Sprintf("unsupported transport type: %s", transport.Type), + ValidationIssueSeverityError, + "unsupported-transport-type", + ) + result.AddIssue(issue) } + + return result } // validateRemoteTransport validates a remote transport (no templating allowed) -func validateRemoteTransport(obj *model.Transport) error { +func validateRemoteTransport(ctx *ValidationContext, obj *model.Transport) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Validate transport type is supported - remotes only support streamable-http and sse switch obj.Type { case model.TransportTypeStreamableHTTP, model.TransportTypeSSE: // URL is required for streamable-http and sse if obj.URL == "" { - return fmt.Errorf("url is required for %s transport type", obj.Type) - } - // Validate URL format (no templates allowed for remotes, no localhost) - if !IsValidRemoteURL(obj.URL) { - return fmt.Errorf("%w: %s", ErrInvalidRemoteURL, obj.URL) + issue := NewValidationIssue( + ValidationIssueTypeSemantic, + ctx.Field("url").String(), + fmt.Sprintf("url is required for %s transport type", obj.Type), + ValidationIssueSeverityError, + "remote-transport-url-required", + ) + result.AddIssue(issue) + } else if !IsValidRemoteURL(obj.URL) { + // Validate URL format (no templates allowed for remotes, no localhost) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Field("url").String(), + fmt.Errorf("%w: %s", ErrInvalidRemoteURL, obj.URL), + "invalid-remote-url", + ) + result.AddIssue(issue) } - return nil default: - return fmt.Errorf("unsupported transport type for remotes: %s (only streamable-http and sse are supported)", obj.Type) + issue := NewValidationIssue( + ValidationIssueTypeSemantic, + ctx.Field("type").String(), + fmt.Sprintf("unsupported transport type for remotes: %s (only streamable-http and sse are supported)", obj.Type), + ValidationIssueSeverityError, + "unsupported-remote-transport-type", + ) + result.AddIssue(issue) } + + return result } // ValidatePublishRequest validates a complete publish request including extensions @@ -511,31 +711,46 @@ func parseServerName(serverJSON apiv0.ServerJSON) (string, error) { } // validateRemoteNamespaceMatch validates that remote URLs match the reverse-DNS namespace -func validateRemoteNamespaceMatch(serverJSON apiv0.ServerJSON) error { +func validateRemoteNamespaceMatch(ctx *ValidationContext, serverJSON apiv0.ServerJSON) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} namespace := serverJSON.Name - for _, remote := range serverJSON.Remotes { + for i, remote := range serverJSON.Remotes { if err := validateRemoteURLMatchesNamespace(remote.URL, namespace); err != nil { - return fmt.Errorf("remote URL %s does not match namespace %s: %w", remote.URL, namespace, err) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.Index(i).Field("url").String(), + fmt.Errorf("remote URL %s does not match namespace %s: %w", remote.URL, namespace, err), + "remote-url-namespace-mismatch", + ) + result.AddIssue(issue) } } - return nil + return result } // validateWebsiteURLNamespaceMatch validates that website URL matches the reverse-DNS namespace -func validateWebsiteURLNamespaceMatch(serverJSON apiv0.ServerJSON) error { +func validateWebsiteURLNamespaceMatch(ctx *ValidationContext, serverJSON apiv0.ServerJSON) *ValidationResult { + result := &ValidationResult{Valid: true, Issues: []ValidationIssue{}} + // Skip validation if website URL is not provided if serverJSON.WebsiteURL == "" { - return nil + return result } namespace := serverJSON.Name if err := validateRemoteURLMatchesNamespace(serverJSON.WebsiteURL, namespace); err != nil { - return fmt.Errorf("websiteUrl %s does not match namespace %s: %w", serverJSON.WebsiteURL, namespace, err) + issue := NewValidationIssueFromError( + ValidationIssueTypeSemantic, + ctx.String(), + fmt.Errorf("websiteUrl %s does not match namespace %s: %w", serverJSON.WebsiteURL, namespace, err), + "website-url-namespace-mismatch", + ) + result.AddIssue(issue) } - return nil + return result } // validateRemoteURLMatchesNamespace checks if a remote URL's hostname matches the publisher domain from the namespace