Skip to content

Conversation

@rchincha
Copy link
Contributor

What type of PR is this?

Which issue does this PR fix:

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rchincha rchincha requested a review from andaaron as a code owner May 20, 2025 20:00
@rchincha rchincha requested review from andaaron and Copilot and removed request for andaaron May 20, 2025 20:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces MCP support for the GraphQL API, adding new endpoints and corresponding tests for MCP query and playground functionality.

  • Adds a new MCP server implementation in pkg/extensions/mcp/server.go
  • Introduces unit tests in pkg/extensions/mcp/server_test.go to validate endpoint responses
  • Integrates MCP endpoints into the main API routes and provides a disabled alternative in pkg/extensions/extension_mcp_disabled.go

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/extensions/mcp/server_test.go Adds tests for both the query and playground endpoints
pkg/extensions/mcp/server.go Implements the MCP GraphQL API endpoints using gqlgen and playground
pkg/extensions/extension_mcp_disabled.go Provides a stub implementation when MCP is not built in
pkg/extensions/extension_mcp.go Registers MCP endpoints on both ServeMux and gorilla/mux routers
pkg/api/routes.go Integrates MCP routes into the main API router

@rchincha rchincha force-pushed the mcp branch 2 times, most recently from 2f2c5ce to 0bd916a Compare May 21, 2025 02:42
@rchincha rchincha requested a review from Copilot May 21, 2025 02:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for the MCP (GraphQL) extension by integrating the MCP-golang server and transport into the Zot registry, registering new routes, and updating build dependencies.

  • Introduces a new HTTP transport (GinTransport) and base JSON-RPC handling under pkg/extensions/mcp
  • Implements NewMCPServer, updates router setup in extension_mcp.go, and adds build-tagged stubs when MCP is disabled
  • Updates go.mod, route constants, and Makefile to include MCP extension

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/extensions/mcp/transport.go HTTP transport for JSON-RPC messages using standard http.Handler
pkg/extensions/mcp/common.go Core baseTransport logic for request/response handling
pkg/extensions/mcp/server.go NewMCPServer factory (currently a no-op)
pkg/extensions/mcp/server_test.go Basic endpoint tests for MCP server
pkg/extensions/extension_mcp.go Conditional registration of MCP routes with CORS/security middleware
pkg/extensions/extension_mcp_disabled.go No-op MCP stubs when build tag !mcp is set
pkg/api/constants/extensions.go New constants for MCP route prefixes
pkg/api/routes.go Adds call to SetupMCPRoutes in main router setup
go.mod Adds github.com/metoro-io/mcp-golang and other deps
Makefile Includes mcp in EXTENSIONS and ALL_EXTENSIONS lists
Comments suppressed due to low confidence (2)

pkg/extensions/extension_mcp.go:29

  • The MCP handler is being registered on extRouter but passed the parent router, so requests under the /mcp prefix will not be routed correctly; pass extRouter to NewMCPServer or return a handler bound to the prefix.
        .Handler(mcp.NewMCPServer(router)) //nolint: staticcheck

pkg/extensions/mcp/server_test.go:10

  • The test calls NewMCPServer() without the required router *mux.Router parameter (signature changed), causing a compile error; update the test to pass a *mux.Router or adjust the function signature.
handler := NewMCPServer()

}

// NewMCPServer returns an HTTP handler for the MCP GraphQL API
func NewMCPServer(router *mux.Router) http.Handler {
Copy link

Copilot AI May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewMCPServer currently returns the passed-in router without mounting the MCP server's handler, so no MCP endpoints are actually registered; update it to attach srv (e.g., via srv.ServeHTTP) to the router or return an appropriate handler.

Copilot uses AI. Check for mistakes.
Comment on lines 10 to 17
type mcpRouter struct {
router *mux.Router
}

func (m *mcpRouter) Close() error {
return nil
}

Copy link

Copilot AI May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mcpRouter type is declared but never used; consider removing it to reduce dead code.

Suggested change
type mcpRouter struct {
router *mux.Router
}
func (m *mcpRouter) Close() error {
return nil
}
// Removed unused mcpRouter type and its Close method.

Copilot uses AI. Check for mistakes.
t.mu.Lock()
var key int64 = 0

for key < 1000000 {
Copy link

Copilot AI May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The hardcoded limit of 1000000 for finding an unused request ID is a magic number; extract it into a named constant or document why this threshold was chosen.

Copilot uses AI. Check for mistakes.
go.mod Outdated
github.com/aws/aws-sdk-go-v2/service/ssooidc v1.30.1 // indirect
github.com/aws/aws-sdk-go-v2/service/sts v1.33.19 // indirect
github.com/awslabs/amazon-ecr-credential-helper/ecr-login v0.9.1 // indirect
github.com/bahlo/generic-list-go v0.2.0 // indirect
Copy link

Copilot AI May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency (generic-list-go) is not referenced in the new code; run go mod tidy to remove unused modules.

Copilot uses AI. Check for mistakes.
@rchincha rchincha marked this pull request as draft May 22, 2025 23:04
@github-actions
Copy link

github-actions bot commented Oct 9, 2025

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

Signed-off-by: Ramkumar Chinchani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant