ronantakizawa/codereview-bench huggingface dataset:
Repository: modelcontextprotocol/registry
File: tools/validate-schemas/main.go (Go)

Code before:
		Short: "Validate JSON schema files",
		Long:  "Validates that schema.json and registry-schema.json are valid JSON Schema documents",
		RunE:  runValidation,
	}

	if err := rootCmd.Execute(); err != nil {
		log.Fatalf("Error: %v", err)
	}
}

func runValidation(_ *cobra.Command, _ []string) error {
	basePath := filepath.Join("docs", "server-json")

	schemas := []struct {
		name string
		path string
	}{
		{"schema.json", filepath.Join(basePath, "schema.json")},
		{"registry-schema.json", filepath.Join(basePath, "registry-schema.json")},
	}

	expectedSchemaCount := len(schemas)
	compiler := jsonschema.NewCompiler()
	compiler.Draft = jsonschema.Draft7

	allValid := true
	validatedCount := 0

	for _, schemaFile := range schemas {
		log.Printf("Validating %s...", schemaFile.name)

		if err := validateSchema(compiler, schemaFile.path); err != nil {
			log.Printf("  ❌ Invalid: %v", err)
			allValid = false
		} else {
			log.Printf("  ✅ Valid JSON Schema")
			validatedCount++
		}
	}

	if !allValid {
		return fmt.Errorf("one or more schemas are invalid")
	}

	if validatedCount != expectedSchemaCount {
		return fmt.Errorf("validation count mismatch: expected to validate %d schemas but only %d passed",
			expectedSchemaCount, validatedCount)
	}

	log.Printf("\nSuccessfully validated all %d schemas!", validatedCount)
	return nil

Review comment (suggestion):
Do you need this? `validatedCount` counts valid schemas, so I expect `allValid == (validatedCount == expectedSchemaCount)`

Code after:
}

func runValidation() error {
	basePath := filepath.Join("docs", "server-json")

	schemas := []struct {
		name string
		path string
	}{
		{"schema.json", filepath.Join(basePath, "schema.json")},
		{"registry-schema.json", filepath.Join(basePath, "registry-schema.json")},
	}

	expectedSchemaCount := len(schemas)
	validatedCount := 0

	for _, schemaFile := range schemas {
		log.Printf("Validating %s...", schemaFile.name)

		if err := validateSchema(schemaFile.path); err != nil {
			log.Printf("  ❌ Invalid: %v", err)
		} else {
			log.Printf("  ✅ Valid JSON Schema")
			validatedCount++
		}
	}

	if validatedCount != expectedSchemaCount {
		return fmt.Errorf("validation failed: expected to validate %d schemas but only %d passed",
			expectedSchemaCount, validatedCount)
	}

	log.Printf("\nSuccessfully validated all %d schemas!", validatedCount)
	return nil
}

func validateSchema(path string) error {
	data, err := os.ReadFile(path)
	if err != nil {
		return fmt.Errorf("failed to read file: %w", err)
	}

	var schemaData any
	if err := json.Unmarshal(data, &schemaData); err != nil {
		return fmt.Errorf("invalid JSON: %w", err)
	}

	compiler := jsonschema.NewCompiler()
	compiler.Draft = jsonschema.Draft7

	// For registry-schema.json, we need to register the base schema it references
Repository: modelcontextprotocol/registry
File: internal/validators/validators_test.go (Go)

Code before:
					},
				},
			},
			expectedError: validators.ErrInvalidRemoteURL.Error(),
		},
		{
			name: "remote with missing scheme",
			serverDetail: apiv0.ServerJSON{
				Name:        "com.example/test-server",
				Description: "A test server",
				Repository: model.Repository{
					URL:    "https://github.com/owner/repo",
					Source: "github",
				},
				VersionDetail: model.VersionDetail{
					Version: "1.0.0",
				},
				Remotes: []model.Remote{
					{
						URL: "example.com/remote",
					},
				},
			},
			expectedError: validators.ErrInvalidRemoteURL.Error(),
		},
		{
			name: "remote with localhost url",
			serverDetail: apiv0.ServerJSON{
				Name:        "com.example/test-server",
				Description: "A test server",
				Repository: model.Repository{
					URL:    "https://github.com/owner/repo",
					Source: "github",
				},
				VersionDetail: model.VersionDetail{
					Version: "1.0.0",
				},
				Remotes: []model.Remote{
					{
						URL: "http://localhost",
					},
				},
			},
			expectedError: validators.ErrInvalidRemoteURL.Error(),
		},
		{
			name: "multiple remotes with one invalid",
			serverDetail: apiv0.ServerJSON{
				Name:        "com.example/test-server",
				Description: "A test server",
				Repository: model.Repository{

Review comment (suggestion):
could we also add a test case for `localhost:8080`? Not sure if this is covered right now, and seems good to catch just in case.

Code after:
					},
				},
			},
			expectedError: validators.ErrInvalidRemoteURL.Error(),
		},
		{
			name: "remote with missing scheme",
			serverDetail: apiv0.ServerJSON{
				Name:        "com.example/test-server",
				Description: "A test server",
				Repository: model.Repository{
					URL:    "https://github.com/owner/repo",
					Source: "github",
				},
				VersionDetail: model.VersionDetail{
					Version: "1.0.0",
				},
				Remotes: []model.Remote{
					{
						URL: "example.com/remote",
					},
				},
			},
			expectedError: validators.ErrInvalidRemoteURL.Error(),
		},
		{
			name: "remote with localhost url",
			serverDetail: apiv0.ServerJSON{
				Name:        "com.example/test-server",
				Description: "A test server",
				Repository: model.Repository{
					URL:    "https://github.com/owner/repo",
					Source: "github",
				},
				VersionDetail: model.VersionDetail{
					Version: "1.0.0",
				},
				Remotes: []model.Remote{
					{
						URL: "http://localhost",
					},
				},
			},
			expectedError: validators.ErrInvalidRemoteURL.Error(),
		},
		{
			name: "remote with localhost url with port",
			serverDetail: apiv0.ServerJSON{
				Name:        "com.example/test-server",
				Description: "A test server",
				Repository: model.Repository{
Repository: modelcontextprotocol/registry
File: cmd/publisher/commands/publish.go (Go)

Code before:
	}
	publishURL := registryURL + "v0/publish"

	// Create and send request
	req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, publishURL, bytes.NewBuffer(jsonData))
	if err != nil {
		return nil, fmt.Errorf("error creating request: %w", err)
	}
	req.Header.Set("Content-Type", "application/json")
	req.Header.Set("Authorization", "Bearer "+token)

	client := &http.Client{}
	resp, err := client.Do(req)
	if err != nil {
		return nil, fmt.Errorf("error sending request: %w", err)
	}
	defer resp.Body.Close()

	// Read response
	body, err := io.ReadAll(resp.Body)
	if err != nil {
		return nil, fmt.Errorf("error reading response: %w", err)
	}

	if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK {
		return &serverJSON, fmt.Errorf("server returned status %d: %s", resp.StatusCode, body)
	}

	if err := json.Unmarshal(body, &serverJSON); err != nil {
		return &serverJSON, err
	}

	return &serverJSON, nil
}


Review comment (suggestion):
```suggestion
		return nil, fmt.Errorf("server returned status %d: %s", resp.StatusCode, body)
```

Code after:
	}
	publishURL := registryURL + "v0/publish"

	// Create and send request
	req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, publishURL, bytes.NewBuffer(jsonData))
	if err != nil {
		return nil, fmt.Errorf("error creating request: %w", err)
	}
	req.Header.Set("Content-Type", "application/json")
	req.Header.Set("Authorization", "Bearer "+token)

	client := &http.Client{}
	resp, err := client.Do(req)
	if err != nil {
		return nil, fmt.Errorf("error sending request: %w", err)
	}
	defer resp.Body.Close()

	// Read response
	body, err := io.ReadAll(resp.Body)
	if err != nil {
		return nil, fmt.Errorf("error reading response: %w", err)
	}

	if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK {
		return nil, fmt.Errorf("server returned status %d: %s", resp.StatusCode, body)
	}

	if err := json.Unmarshal(body, &serverJSON); err != nil {
		return nil, err
	}

	return &serverJSON, nil
}
Repository: modelcontextprotocol/registry
File: internal/database/postgres.go (Go)

Code before:
	if err != nil {
		return nil, fmt.Errorf("failed to marshal server JSON: %w", err)
	}

	// Insert into simple servers table
	query := `
		INSERT INTO servers (id, value)
		VALUES ($1, $2)
	`

	_, err = db.conn.Exec(ctx, query, id, valueJSON)
	if err != nil {
		return nil, fmt.Errorf("failed to insert server: %w", err)
	}

	return server, nil
}

// UpdateServer updates an existing server record with new server details
func (db *PostgreSQL) UpdateServer(ctx context.Context, id string, server *apiv0.ServerJSON) (*apiv0.ServerJSON, error) {
	if ctx.Err() != nil {
		return nil, ctx.Err()
	}

	// Marshal updated server
	// keep the id invariant: the json meta id must match the primary key id
	if server.Meta == nil {
		server.Meta = &apiv0.ServerMeta{}
	}
	if server.Meta.Official == nil {
		server.Meta.Official = &apiv0.RegistryExtensions{}
	}
	server.Meta.Official.ID = id

	valueJSON, err := json.Marshal(server)
	if err != nil {
		return nil, fmt.Errorf("failed to marshal updated server: %w", err)
	}

	// Update the complete server record in simple table
	query := `
		UPDATE servers 
		SET value = $1
		WHERE id = $2
	`

	result, err := db.conn.Exec(ctx, query, valueJSON, id)
	if err != nil {
		return nil, fmt.Errorf("failed to update server: %w", err)
	}


Review comment (bug):
I think I'd probably prefer throwing an error if someone tries to edit an ID - that seems unintentional and likely indicative of a bug/someone misusing this elsewhere. And I worry this might mask things like messing with meta properties accidentally, which we should be very cautious of for the integrity of data in the registry.

i.e. [fail fast principle](https://martinfowler.com/ieeeSoftware/failFast.pdf)

Code after:
	if err != nil {
		return nil, fmt.Errorf("failed to marshal server JSON: %w", err)
	}

	// Insert into simple servers table
	query := `
		INSERT INTO servers (id, value)
		VALUES ($1, $2)
	`

	_, err = db.conn.Exec(ctx, query, id, valueJSON)
	if err != nil {
		return nil, fmt.Errorf("failed to insert server: %w", err)
	}

	return server, nil
}

// UpdateServer updates an existing server record with new server details
func (db *PostgreSQL) UpdateServer(ctx context.Context, id string, server *apiv0.ServerJSON) (*apiv0.ServerJSON, error) {
	if ctx.Err() != nil {
		return nil, ctx.Err()
	}

	// Validate that meta structure exists and ID matches path
	if server.Meta == nil || server.Meta.Official == nil || server.Meta.Official.ID != id {
		return nil, fmt.Errorf("%w: io.modelcontextprotocol.registry/official.id must match path id (%s)", ErrInvalidInput, id)
	}

	// Marshal updated server
	valueJSON, err := json.Marshal(server)
	if err != nil {
		return nil, fmt.Errorf("failed to marshal updated server: %w", err)
	}

	// Update the complete server record in simple table
	query := `
		UPDATE servers 
		SET value = $1
		WHERE id = $2
	`

	result, err := db.conn.Exec(ctx, query, valueJSON, id)
	if err != nil {
		return nil, fmt.Errorf("failed to update server: %w", err)
	}

	if result.RowsAffected() == 0 {
		return nil, ErrNotFound
	}
Repository: modelcontextprotocol/registry
File: internal/service/registry_service.go (Go)

Code before:
	}

	// Validate the request
	if err := validators.ValidatePublishRequest(req, s.cfg); err != nil {
		return nil, err
	}

	// Merge the request with the current server, preserving metadata
	updatedServer := *currentServer // Copy the current server with all metadata

	// Update only the user-modifiable fields from the request
	updatedServer.Name = req.Name
	updatedServer.Description = req.Description
	updatedServer.Version = req.Version
	updatedServer.Status = req.Status
	updatedServer.Repository = req.Repository
	updatedServer.Remotes = req.Remotes
	updatedServer.Packages = req.Packages

	// Update the UpdatedAt timestamp in metadata
	if updatedServer.Meta != nil && updatedServer.Meta.Official != nil {
		updatedServer.Meta.Official.UpdatedAt = time.Now()
	}

	// Check for duplicate remote URLs using the updated server
	if err := s.validateNoDuplicateRemoteURLs(ctx, nil, updatedServer); err != nil {
		return nil, err
	}

	// Update server in database
	serverRecord, err := s.db.UpdateServer(ctx, nil, versionID, &updatedServer)
	if err != nil {
		return nil, err
	}

	// Return the server record directly
	return serverRecord, nil
}


Review comment (refactor):
The fact we use the same `validateNoDuplicateRemoteURLs` function made me think that the EditServer action could modify the server in such a way that it could lead to the same problem being fixed in the Publish endpoint. Is that at all possible?

Code after:

	return database.InTransactionT(ctx, s.db, func(txCtx context.Context, tx pgx.Tx) (*apiv0.ServerJSON, error) {
		// Get the current server to preserve metadata
		currentServer, err := s.db.GetByVersionID(txCtx, tx, versionID)
		if err != nil {
			return nil, err
		}

		// Acquire advisory lock to prevent concurrent edits of servers with same name
		if err := s.db.AcquirePublishLock(txCtx, tx, currentServer.Name); err != nil {
			return nil, err
		}

		// Merge the request with the current server, preserving metadata
		updatedServer := *currentServer

		// Update only the user-modifiable fields from the request
		updatedServer.Name = req.Name
		updatedServer.Description = req.Description
		updatedServer.Version = req.Version
		updatedServer.Status = req.Status
		updatedServer.Repository = req.Repository
		updatedServer.Remotes = req.Remotes
		updatedServer.Packages = req.Packages

		// Update the UpdatedAt timestamp in metadata
		if updatedServer.Meta != nil && updatedServer.Meta.Official != nil {
			updatedServer.Meta.Official.UpdatedAt = time.Now()
		}

		// Check for duplicate remote URLs using the updated server
		if err := s.validateNoDuplicateRemoteURLs(txCtx, tx, updatedServer); err != nil {
			return nil, err
		}

		// Update server in database
		return s.db.UpdateServer(txCtx, tx, versionID, &updatedServer)
	})
}
Repository: modelcontextprotocol/registry
File: cmd/publisher/commands/init.go (Go)

Code before:
	name := getNameFromPackageJSON()
	if name != "" {
		return name
	}

	// Use current directory name as fallback
	if cwd, err := os.Getwd(); err == nil {
		return fmt.Sprintf("com.example/%s", filepath.Base(cwd))
	}

	return "com.example/my-mcp-server"
}

func buildGitHubServerName(repoURL, subfolder string) string {
	parts := strings.Split(repoURL, "/")
	if len(parts) < 5 {
		return ""
	}

	owner := parts[3]
	repo := strings.TrimSuffix(parts[4], ".git")

	// If we're in a subdirectory, use the current folder name
	if subfolder != "" {
		if cwd, err := os.Getwd(); err == nil {
			folderName := filepath.Base(cwd)
			return fmt.Sprintf("io.github.%s/%s", owner, folderName)
		}
	}

	return fmt.Sprintf("io.github.%s/%s", owner, repo)
}

func detectDescription() string {
	// Try to get from package.json
	if data, err := os.ReadFile("package.json"); err == nil {
		var pkg map[string]any
		if json.Unmarshal(data, &pkg) == nil {
			if desc, ok := pkg["description"].(string); ok && desc != "" {
				return desc
			}
		}
	}

	return "An MCP server that provides [describe what your server does]"
}

func detectRepoURL() string {
	// Try git remote
	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	defer cancel()

Review comment (suggestion):
It seems we use `subfolder` as a boolean even though it's a string. Wouldn't it be better to reuse the subfolder variable we already have? 

For example if it's != "" construct the path using it (and avoid reading the base path again), otherwise proceed as if we are in the root repo.

Code after:
	name := getNameFromPackageJSON()
	if name != "" {
		return name
	}

	// Use current directory name as fallback
	if cwd, err := os.Getwd(); err == nil {
		return fmt.Sprintf("com.example/%s", filepath.Base(cwd))
	}

	return "com.example/my-mcp-server"
}

func buildGitHubServerName(repoURL, subfolder string) string {
	parts := strings.Split(repoURL, "/")
	if len(parts) < 5 {
		return ""
	}

	owner := parts[3]
	repo := strings.TrimSuffix(parts[4], ".git")

	// If we're in a subdirectory, use the current folder name
	if subfolder != "" {
		folderName := filepath.Base(subfolder)
		return fmt.Sprintf("io.github.%s/%s", owner, folderName)
	}

	return fmt.Sprintf("io.github.%s/%s", owner, repo)
}

func detectDescription() string {
	// Try to get from package.json
	if data, err := os.ReadFile("package.json"); err == nil {
		var pkg map[string]any
		if json.Unmarshal(data, &pkg) == nil {
			if desc, ok := pkg["description"].(string); ok && desc != "" {
				return desc
			}
		}
	}

	return "An MCP server that provides [describe what your server does]"
}

func detectRepoURL() string {
	// Try git remote
	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	defer cancel()
	cmd := exec.CommandContext(ctx, "git", "remote", "get-url", "origin")
	if output, err := cmd.Output(); err == nil {
Repository: modelcontextprotocol/registry
File: internal/validators/validators.go (Go)

Code before:

	// Ensure it's an absolute URL with valid scheme
	if !parsedURL.IsAbs() {
		return fmt.Errorf("websiteUrl must be absolute (include scheme): %s", websiteURL)
	}

	// Only allow HTTP/HTTPS schemes for security
	if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
		return fmt.Errorf("websiteUrl must use http or https scheme: %s", websiteURL)
	}

	return nil
}

func validateTitle(title string) error {
	// Skip validation if title is not provided (optional field)
	if title == "" {
		return nil
	}

	// Check that title is not only whitespace
	if strings.TrimSpace(title) == "" {
		return fmt.Errorf("title cannot be only whitespace")
	}

	// Check for MCP suffix variants (case-insensitive)
	titleLower := strings.ToLower(strings.TrimSpace(title))
	invalidSuffixes := []string{
		" mcp server",
		" mcp",
		"-mcp server",
		"-mcp",
	}

	for _, suffix := range invalidSuffixes {
		if strings.HasSuffix(titleLower, suffix) {
			return fmt.Errorf("%w: found '%s' suffix", ErrTitleHasMCPSuffix, suffix)
		}
	}

	return nil
}

func validatePackageField(obj *model.Package) error {
	if !HasNoSpaces(obj.Identifier) {
		return ErrPackageNameHasSpaces
	}

	// Validate version string
	if err := validateVersion(obj.Version); err != nil {
		return err

Review comment (suggestion):
Depending on why the MCP suffix is not allowed, it might make sense to block `mcp` as a word completely in the suffix, ignoring the server? So if it says `mcp srvr` or `mcp sv` or `mcp any_word` we'd also block it*

Code after:
	// Skip validation if website URL is not provided (optional field)
	if websiteURL == "" {
		return nil
	}

	// Parse the URL to ensure it's valid
	parsedURL, err := url.Parse(websiteURL)
	if err != nil {
		return fmt.Errorf("invalid websiteUrl: %w", err)
	}

	// Ensure it's an absolute URL with valid scheme
	if !parsedURL.IsAbs() {
		return fmt.Errorf("websiteUrl must be absolute (include scheme): %s", websiteURL)
	}

	// Only allow HTTP/HTTPS schemes for security
	if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
		return fmt.Errorf("websiteUrl must use http or https scheme: %s", websiteURL)
	}

	return nil
}

func validateTitle(title string) error {
	// Skip validation if title is not provided (optional field)
	if title == "" {
		return nil
	}

	// Check that title is not only whitespace
	if strings.TrimSpace(title) == "" {
		return fmt.Errorf("title cannot be only whitespace")
	}

	return nil
}

func validatePackageField(obj *model.Package) error {
	if !HasNoSpaces(obj.Identifier) {
		return ErrPackageNameHasSpaces
	}

	// Validate version string
	if err := validateVersion(obj.Version); err != nil {
		return err
	}

	// Validate runtime arguments
	for _, arg := range obj.RuntimeArguments {
		if err := validateArgument(&arg); err != nil {
Repository: moeru-ai/airi
File: packages/stage-ui/src/composables/live2d/animation.ts (TypeScript)

Code before:

import { randomSaccadeInterval } from '../../utils'

/**
 * This is to simulate idle eye saccades and focus (head) movements in a *pretty* naive way.
 * Not using any reactivity here as it's not yet needed.
 * Keeping it here as a composable for future extension.
 */
export function useLive2DIdleEyeFocus() {
  let nextSaccadeAfter = -1
  let focusTarget: [number, number] | undefined
  let lastSaccadeAt = -1

  // Function to handle idle eye saccades and focus (head) movements
  function update(model: InternalModel, now: number) {
    if (now >= nextSaccadeAfter || now < lastSaccadeAt) {
      focusTarget = [randFloat(-1, 1), randFloat(-1, 0.7)]
      lastSaccadeAt = now
      nextSaccadeAfter = now + (randomSaccadeInterval() / 1000)
      model.focusController.focus(focusTarget![0] * 0.5, focusTarget![1] * 0.5, false)
    }

    model.focusController.update(now - lastSaccadeAt)
    const coreModel = model.coreModel as any
    coreModel.setParameterValueById('ParamEyeBallX', lerp(coreModel.getParameterValueById('ParamEyeBallX'), focusTarget![0], 0.3))
    coreModel.setParameterValueById('ParamEyeBallY', lerp(coreModel.getParameterValueById('ParamEyeBallY'), focusTarget![1], 0.3))
  }

  return { update }
}


Review comment (suggestion):
```suggestion
    // TODO: After emotion mapper, stage editor, eye related parameters should be take cared to be dynamical instead of hardcoding
    coreModel.setParameterValueById('ParamEyeBallX', lerp(coreModel.getParameterValueById('ParamEyeBallX'), focusTarget![0], 0.3))
```

Code after:

import { randomSaccadeInterval } from '../../utils'

/**
 * This is to simulate idle eye saccades and focus (head) movements in a *pretty* naive way.
 * Not using any reactivity here as it's not yet needed.
 * Keeping it here as a composable for future extension.
 */
export function useLive2DIdleEyeFocus() {
  let nextSaccadeAfter = -1
  let focusTarget: [number, number] | undefined
  let lastSaccadeAt = -1

  // Function to handle idle eye saccades and focus (head) movements
  function update(model: InternalModel, now: number) {
    if (now >= nextSaccadeAfter || now < lastSaccadeAt) {
      focusTarget = [randFloat(-1, 1), randFloat(-1, 0.7)]
      lastSaccadeAt = now
      nextSaccadeAfter = now + (randomSaccadeInterval() / 1000)
      model.focusController.focus(focusTarget![0] * 0.5, focusTarget![1] * 0.5, false)
    }

    model.focusController.update(now - lastSaccadeAt)
    const coreModel = model.coreModel as any
    // TODO: After emotion mapper, stage editor, eye related parameters should be take cared to be dynamical instead of hardcoding
    coreModel.setParameterValueById('ParamEyeBallX', lerp(coreModel.getParameterValueById('ParamEyeBallX'), focusTarget![0], 0.3))
    coreModel.setParameterValueById('ParamEyeBallY', lerp(coreModel.getParameterValueById('ParamEyeBallY'), focusTarget![1], 0.3))
  }

  return { update }
}