Skip to content

repository resource tests #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 2, 2025
Merged

repository resource tests #69

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
383c2d2
refactor to make testing easier
mntlty Apr 2, 2025
1cb52f9
not needed in handler func
mntlty Apr 2, 2025
2aa3002
small cleanup
mntlty Apr 2, 2025
d8b0056
create repository_resource_test
mntlty Apr 2, 2025
2fdda7c
remove chatty comments
mntlty Apr 2, 2025
9680b24
comment cleanup, function rename and some more tests
mntlty Apr 2, 2025
b4e6772
fix test for ubuntu runner
mntlty Apr 2, 2025
db7a180
remove it for now
mntlty Apr 2, 2025
02ebdc7
make required args explicit instead of panic
mntlty Apr 2, 2025
ad58220
more tests and cleanup
mntlty Apr 2, 2025
ede9f22
chore: use raw repo resources (#70)
SamMorrowDrums Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 99 additions & 78 deletions pkg/github/repository_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,110 +13,131 @@ import (
"github.com/mark3labs/mcp-go/server"
)

// getRepositoryContent defines the resource template and handler for the Repository Content API.
func getRepositoryContent(client *github.Client, t translations.TranslationHelperFunc) (mainTemplate mcp.ResourceTemplate, reftemplate mcp.ResourceTemplate, shaTemplate mcp.ResourceTemplate, tagTemplate mcp.ResourceTemplate, prTemplate mcp.ResourceTemplate, handler server.ResourceTemplateHandlerFunc) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this format of returning multiple values made it quite hard to evaluate and test, instead I broke them up into individual functions


// getRepositoryResourceContent defines the resource template and handler for getting repository content.
func getRepositoryResourceContent(client *github.Client, t translations.TranslationHelperFunc) (mcp.ResourceTemplate, server.ResourceTemplateHandlerFunc) {
return mcp.NewResourceTemplate(
"repo://{owner}/{repo}/contents{/path*}", // Resource template
t("RESOURCE_REPOSITORY_CONTENT_DESCRIPTION", "Repository Content"),
), mcp.NewResourceTemplate(
),
repositoryResourceContentsHandler(client)
}

// getRepositoryContent defines the resource template and handler for getting repository content for a branch.
func getRepositoryResourceBranchContent(client *github.Client, t translations.TranslationHelperFunc) (mcp.ResourceTemplate, server.ResourceTemplateHandlerFunc) {
return mcp.NewResourceTemplate(
"repo://{owner}/{repo}/refs/heads/{branch}/contents{/path*}", // Resource template
t("RESOURCE_REPOSITORY_CONTENT_BRANCH_DESCRIPTION", "Repository Content for specific branch"),
), mcp.NewResourceTemplate(
),
repositoryResourceContentsHandler(client)
}

// getRepositoryResourceCommitContent defines the resource template and handler for getting repository content for a commit.
func getRepositoryResourceCommitContent(client *github.Client, t translations.TranslationHelperFunc) (mcp.ResourceTemplate, server.ResourceTemplateHandlerFunc) {
return mcp.NewResourceTemplate(
"repo://{owner}/{repo}/sha/{sha}/contents{/path*}", // Resource template
t("RESOURCE_REPOSITORY_CONTENT_COMMIT_DESCRIPTION", "Repository Content for specific commit"),
), mcp.NewResourceTemplate(
),
repositoryResourceContentsHandler(client)
}

// getRepositoryResourceTagContent defines the resource template and handler for getting repository content for a tag.
func getRepositoryResourceTagContent(client *github.Client, t translations.TranslationHelperFunc) (mcp.ResourceTemplate, server.ResourceTemplateHandlerFunc) {
return mcp.NewResourceTemplate(
"repo://{owner}/{repo}/refs/tags/{tag}/contents{/path*}", // Resource template
t("RESOURCE_REPOSITORY_CONTENT_TAG_DESCRIPTION", "Repository Content for specific tag"),
), mcp.NewResourceTemplate(
),
repositoryResourceContentsHandler(client)
}

// getRepositoryResourcePrContent defines the resource template and handler for getting repository content for a pull request.
func getRepositoryResourcePrContent(client *github.Client, t translations.TranslationHelperFunc) (mcp.ResourceTemplate, server.ResourceTemplateHandlerFunc) {
return mcp.NewResourceTemplate(
"repo://{owner}/{repo}/refs/pull/{pr_number}/head/contents{/path*}", // Resource template
t("RESOURCE_REPOSITORY_CONTENT_PR_DESCRIPTION", "Repository Content for specific pull request"),
), func(ctx context.Context, request mcp.ReadResourceRequest) ([]mcp.ResourceContents, error) {
// Extract parameters from request.Params.URI
),
repositoryResourceContentsHandler(client)
}

owner := request.Params.Arguments["owner"].([]string)[0]
repo := request.Params.Arguments["repo"].([]string)[0]
// path should be a joined list of the path parts
path := strings.Join(request.Params.Arguments["path"].([]string), "/")
func repositoryResourceContentsHandler(client *github.Client) func(ctx context.Context, request mcp.ReadResourceRequest) ([]mcp.ResourceContents, error) {
return func(ctx context.Context, request mcp.ReadResourceRequest) ([]mcp.ResourceContents, error) {

opts := &github.RepositoryContentGetOptions{}
owner := request.Params.Arguments["owner"].([]string)[0]
repo := request.Params.Arguments["repo"].([]string)[0]
// path should be a joined list of the path parts
path := strings.Join(request.Params.Arguments["path"].([]string), "/")

sha, ok := request.Params.Arguments["sha"].([]string)
if ok {
opts.Ref = sha[0]
}
opts := &github.RepositoryContentGetOptions{}

branch, ok := request.Params.Arguments["branch"].([]string)
if ok {
opts.Ref = "refs/heads/" + branch[0]
}
sha, ok := request.Params.Arguments["sha"].([]string)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

left this as it was previously, where one function was shared by all ResourceTemplates. I think it would be better to continue the refactor by creating individual functions that only parse arguments that are expected, for example the sha argument would only be parsed when getting a commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I just wanted to quickly scale out what I had written to multiple endpoints - but in truth, the shared logic could be shared, with separate opts generation and that is likely a better direction.

tl;dr I think you're right. I just spent enough time trying to decide how to wrap it as a resource, and make it work, I didn't spend much time on how it should be written.

if ok {
opts.Ref = sha[0]
}

tag, ok := request.Params.Arguments["tag"].([]string)
if ok {
opts.Ref = "refs/tags/" + tag[0]
}
prNumber, ok := request.Params.Arguments["pr_number"].([]string)
if ok {
opts.Ref = "refs/pull/" + prNumber[0] + "/head"
}
branch, ok := request.Params.Arguments["branch"].([]string)
if ok {
opts.Ref = "refs/heads/" + branch[0]
}

// Use the GitHub client to fetch repository content
fileContent, directoryContent, _, err := client.Repositories.GetContents(ctx, owner, repo, path, opts)
if err != nil {
return nil, err
}
tag, ok := request.Params.Arguments["tag"].([]string)
if ok {
opts.Ref = "refs/tags/" + tag[0]
}
prNumber, ok := request.Params.Arguments["pr_number"].([]string)
if ok {
opts.Ref = "refs/pull/" + prNumber[0] + "/head"
}

if directoryContent != nil {
// Process the directory content and return it as resource contents
var resources []mcp.ResourceContents
for _, entry := range directoryContent {
mimeType := "text/directory"
if entry.GetType() == "file" {
mimeType = mime.TypeByExtension(filepath.Ext(entry.GetName()))
}
resources = append(resources, mcp.TextResourceContents{
URI: entry.GetHTMLURL(),
MIMEType: mimeType,
Text: entry.GetName(),
})
fileContent, directoryContent, _, err := client.Repositories.GetContents(ctx, owner, repo, path, opts)
if err != nil {
return nil, err
}

if directoryContent != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure if directoryContent and fileContent are mutually exclusive, that is the previous behavior which I haven't changed (I only removed the unnecessary else clause)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are only returning the names of things with directoryContent, and not the actual file data. When we are returning the file data, we actually serialise the payload.

That's the best I could come up with, the alternative would be to not have a way to inspect directory content, and just return the fact it's a directory.

It's not a perfect fit, but no way are we returning all the blobs in a folder 😅

var resources []mcp.ResourceContents
for _, entry := range directoryContent {
mimeType := "text/directory"
if entry.GetType() == "file" {
mimeType = mime.TypeByExtension(filepath.Ext(entry.GetName()))
}
resources = append(resources, mcp.TextResourceContents{
URI: entry.GetHTMLURL(),
MIMEType: mimeType,
Text: entry.GetName(),
})

}
return resources, nil

}
if fileContent != nil {
if fileContent.Content != nil {
decodedContent, err := fileContent.GetContent()
if err != nil {
return nil, err
}
return resources, nil

} else if fileContent != nil {
// Process the file content and return it as a binary resource

if fileContent.Content != nil {
decodedContent, err := fileContent.GetContent()
if err != nil {
return nil, err
}

mimeType := mime.TypeByExtension(filepath.Ext(fileContent.GetName()))

// Check if the file is text-based
if strings.HasPrefix(mimeType, "text") {
// Return as TextResourceContents
return []mcp.ResourceContents{
mcp.TextResourceContents{
URI: request.Params.URI,
MIMEType: mimeType,
Text: decodedContent,
},
}, nil
}

// Otherwise, return as BlobResourceContents

mimeType := mime.TypeByExtension(filepath.Ext(fileContent.GetName()))

if strings.HasPrefix(mimeType, "text") {
return []mcp.ResourceContents{
mcp.BlobResourceContents{
mcp.TextResourceContents{
URI: request.Params.URI,
MIMEType: mimeType,
Blob: base64.StdEncoding.EncodeToString([]byte(decodedContent)), // Encode content as Base64
Text: decodedContent,
},
}, nil
}
}

return nil, nil
return []mcp.ResourceContents{
mcp.BlobResourceContents{
URI: request.Params.URI,
MIMEType: mimeType,
Blob: base64.StdEncoding.EncodeToString([]byte(decodedContent)), // Encode content as Base64
},
}, nil
}
}

return nil, nil
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a test for this behavior, but it seems wrong - I feel like we should be returning an error here

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are correct, and we shouldn't actually be able to hit it in practice, because the real server would falter here:

https://github.com/github/github-mcp-server/pull/69/files#diff-8c2319afb75694e93b084f504d1c1fc8de262f4301c1888c05ab067d5bf0a21bR91

So this should be one of those if you are here, there be dragons kind of something went wrong errors.

}
}
Loading
Loading