Rebase the mcp/registry dependency to v1.3.3

[rdimitrov]

The following PR:

• Rebases to the code to the latest registry release
• Updates the validation we do on the generated entries + the whole registry (now that the registry hosts the reference schema this is much easier). Added also a unit test to ensure the registry schema matches the version of the server json schema in the version of the registry we depend on
• Added the target_port field to several servers (it was missing, but required in the upstream format)
• Simplified the OCI identifier to use full image reference (yey)
• Removed the registry extensions since they are no longer part of the server object

[github-actions]

🔧 MCP Server Tool List Updates

The tool lists for modified MCP server specs have been automatically updated using thv mcp list.

Summary

Server	Status	Details
No changes detected

---

This comment is automatically generated and will be updated as the workflow progresses.

[JAORMX]

@claude please review this

[rdimitrov]

@claude - please review this

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[stacklokbot]

@claude - please review this

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[rdimitrov]

@claude - please review this

[claude]

Claude finished @rdimitrov's task —— View job

---

Review Progress

• Read repository guidelines (CLAUDE.md)
• Examine PR changes overview
• Review converter code organization
• Review schema and validation changes
• Review registry spec changes (target_port)
• Check test coverage
• Provide comprehensive feedback

---

Code Review

This PR successfully rebases to registry v1.3.3 and makes significant improvements to the registry infrastructure. Overall, the changes are well-structured and properly tested. Here are my findings:

✅ Strengths

1. Excellent Code Organization (pkg/registry/converters/)

   • The converter code has been properly split into separate files as requested:
     • toolhive_to_upstream.go - Clear conversion from ToolHive to upstream format
     • upstream_to_toolhive.go - Clear conversion from upstream to ToolHive format
     • utils.go - Shared utility functions
   • This separation makes the codebase much more maintainable

2. Comprehensive Test Coverage (1,935+ lines of tests)

   • converters_test.go - Unit tests for individual conversion functions
   • integration_test.go - Integration tests for round-trip conversions
   • converters_fixture_test.go - Fixture-based tests with real-world examples
   • Extensive test data in testdata/ directory with clear documentation

3. Smart Schema Validation (pkg/registry/schema_version_test.go:13-82)

   • New test ensures schema version stays in sync with the registry dependency
   • Eliminates the need for manual schema syncing scripts
   • Uses model.CurrentSchemaURL directly, which is a much cleaner approach

4. Simplified Schema Management

   • Removed the complex sync-schema-version.sh script (218 lines deleted)
   • Updated schema reference to use stable URL: https://static.modelcontextprotocol.io/schemas/2025-10-17/server.schema.json
   • Cleaner Renovate configuration without post-upgrade tasks

5. Go Version Resolution

   • Successfully downgraded to go 1.24.6 to match UBI constraints (addressed in review comments)

⚠️ Issues & Concerns

1. Port Conflict Risk with target_port (Critical)

The addition of target_port: 8080 to multiple servers creates a significant port conflict issue:

# Multiple servers now have the same target_port
registry/dolt/spec.yaml:target_port: 8080
registry/fetch/spec.yaml:target_port: 8080
registry/k8s/spec.yaml:target_port: 8080
registry/oci-registry/spec.yaml:target_port: 8080
registry/osv/spec.yaml:target_port: 8080

Problem: Based on the review discussion (particularly JAORMX's comment), ToolHive will use the specified port, and if it's not available, it will fail rather than choosing a random port.

Impact: Users won't be able to run multiple HTTP/SSE-based MCP servers simultaneously if they all specify the same target_port.

Recommendation:

• For Stacklok MCP servers that support the MCP_PORT environment variable (like oci-registry), consider either:
  • Omitting target_port entirely to let ToolHive assign ports dynamically, OR
  • Assigning unique default ports to each server, OR
  • Documenting that the MCP_PORT env var can override the target port

This needs clarification on the intended behavior before merge.

2. Schema Version Metadata Mismatch

In schemas/registry.schema.json:64-68, the metadata shows:

"mcp_registry_version": "v1.3.3",
"schema_date": "2025-10-17",

But go.mod:7 shows:

github.com/modelcontextprotocol/registry v1.3.5

The dependency is at v1.3.5, not v1.3.3. The schema metadata should be updated to match.

3. Port Extraction Logic (pkg/registry/converters/upstream_to_toolhive.go:74-83)

The port extraction from transport URL is solid, but there's a subtle issue:

if pkg.Transport.URL != "" {
    parsedURL, err := url.Parse(pkg.Transport.URL)
    if err == nil && parsedURL.Port() != "" {
        if port, err := strconv.Atoi(parsedURL.Port()); err == nil {
            imageMetadata.TargetPort = port
        }
    }
}

This silently ignores parse errors. Consider logging or returning errors for invalid URLs to help with debugging.

4. Fallback Repository URL (pkg/registry/official.go:236-238)

if repositoryURL == "" {
    repositoryURL = "https://github.com/stacklok/toolhive-registry"

Using a placeholder URL for remote servers without a repository is reasonable, but this could be confusing. Consider:

• Adding a comment explaining why this is needed
• Using a more explicit placeholder format (e.g., https://github.com/stacklok/toolhive-registry/registry/[server-name])

5. TODO Comments (Need Addressing)

Two TODO comments in toolhive_to_upstream.go:

• Line 29: Version: "1.0.0", // TODO: Extract from image tag or metadata
• Line 67: Version: "1.0.0", // TODO: Version management

These should be tracked. Are there plans to implement proper version extraction?

📝 Minor Issues

1. Documentation Gap: The new converter package would benefit from a package-level doc comment explaining the bidirectional conversion approach

2. Error Message Clarity (pkg/registry/converters/upstream_to_toolhive.go:41):

   return nil, fmt.Errorf("serverJSON has %d OCI packages, expected exactly 1", len(ociPackages))

   Consider adding which packages were found to help with debugging

3. Magic Number: transport.URL = "http://localhost" (line 124) - This assumes localhost, but should this be configurable?

🔍 Architecture Questions

1. Extension Key Strategy: Both converters use io.github.stacklok as the extension namespace and nest data under image reference or URL. Is this namespace registered/claimed with the MCP registry community?

2. PackageArguments vs Args: The code converts structured PackageArguments to flat Args strings (upstream_to_toolhive.go:299-314). Is there a risk of data loss in complex argument scenarios?

3. Schema Validation Placement: The validation is now in WriteJSON which is good, but should there also be validation when loading entries from YAML?

---

Summary

This is a solid refactoring that modernizes the registry infrastructure. The main blocker is the target_port port conflict issue which needs resolution based on the review discussion. The schema version mismatch should also be corrected.

Recommendation: Request clarification on the target_port behavior and resolve the schema version mismatch before merging.

---

[rdimitrov]

@claude - can you review it again to make sure the feedback from your previous comment was addressed. Note that the one about the target_port is not a problem for now.

[claude]

Claude finished @rdimitrov's task —— View job

---

Follow-Up Review - Complete ✅

---

Summary

Great work addressing the feedback! After reviewing the changes, I can confirm that all critical and major issues from the previous review have been addressed. The PR is in much better shape now.

---

✅ Issues Successfully Addressed

1. Schema Version Mismatch - FIXED ✅

• Previous issue: Schema metadata showed v1.3.3 but go.mod had v1.3.5
• Status: Fixed in schemas/registry.schema.json:64 - now correctly shows "mcp_registry_version": "v1.3.5"

2. Port Extraction Logic - FIXED ✅

• Previous issue: Port extraction silently ignored parse errors
• Status: Fixed in pkg/registry/converters/upstream_to_toolhive.go:80-91
  • Now logs parse errors with clear warning messages
  • Uses fmt.Printf with emoji warnings for easy identification
  • Includes server name and URL in error messages for debugging
  • Doesn't fail the conversion, which is the correct behavior

3. Fallback Repository URL Documentation - FIXED ✅

• Previous issue: Placeholder URL was confusing without explanation
• Status: Excellent documentation added in multiple locations:
  • pkg/registry/converters/toolhive_to_upstream.go:47-56 (image metadata)
  • pkg/registry/converters/toolhive_to_upstream.go:96-105 (remote metadata)
  • pkg/registry/official.go:236-240
  • Clear explanation of why it's necessary (struct vs pointer, JSON serialization, URI validation)

4. Code Organization - FIXED ✅

• Previous issue: Converter code in single file
• Status: Properly split into separate files:
  • toolhive_to_upstream.go - ToolHive → upstream conversions
  • upstream_to_toolhive.go - upstream → ToolHive conversions
  • utils.go - Shared utility functions
  • Added comprehensive package-level documentation

5. Go Version Compatibility - FIXED ✅

• Previous issue: Go 1.25 incompatible with UBI
• Status: Downgraded to go 1.24.6 in go.mod:3

6. Transport URL Localhost Documentation - FIXED ✅

• Previous issue: Magic "http://localhost" value in line 124
• Status: Excellent comment added in pkg/registry/converters/toolhive_to_upstream.go:148-150 explaining:
  • Why localhost is used (port forwarding from containers)
  • How container networking works
  • Why clients connect via localhost on host

---

📝 Remaining Items (Non-Blocking)

1. TODO Comments - Status: Acknowledged ⚠️

Two TODO comments remain in toolhive_to_upstream.go:

• Line 37: Version: "1.0.0", // TODO: Extract from image tag or metadata
• Line 86: Version: "1.0.0", // TODO: Version management

Impact: Low - These are future enhancements, not bugs
Recommendation: These are fine to leave as-is if there are no immediate plans to implement version extraction. Consider creating a GitHub issue to track this future work if it's a priority.

2. Port Conflict with target_port - Status: Acknowledged by User ✅

As requested, I'm not flagging this as a problem since you noted "the one about the target_port is not a problem for now."

For future reference, the current state is:

• Multiple servers have target_port: 8080 (dolt, fetch, k8s, oci-registry, osv)
• Users won't be able to run multiple HTTP/SSE servers simultaneously with the same port
• This can be addressed later if needed via MCP_PORT environment variable

---

🎯 Architecture Improvements Observed

1. Excellent Test Coverage:

   • Unit tests (converters_test.go)
   • Integration tests (integration_test.go) with round-trip validation
   • Fixture-based tests with real-world examples
   • Well-documented test data in testdata/README.md

2. Smart Schema Validation (schema_version_test.go):

   • Ensures schema stays in sync with registry dependency
   • Eliminates need for manual sync scripts
   • Uses model.CurrentSchemaURL directly

3. Clean Extension Namespace:

   • Uses io.github.stacklok consistently
   • Properly structured extension data

4. Bidirectional Conversion:

   • Clean separation of concerns
   • Proper error handling
   • Good fallback behavior

---

Final Recommendation

LGTM (Looks Good To Me) - This PR is ready to merge! 🎉

All critical feedback has been addressed:

• ✅ Schema version corrected
• ✅ Error logging improved
• ✅ Documentation added
• ✅ Code organization enhanced
• ✅ Go version compatibility fixed
• ✅ Comprehensive test coverage

The TODO comments are minor future enhancements that don't block the current work. The port conflict issue is acknowledged and can be addressed later if needed.

---