Skip to content
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.

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

total glow-up: github MCP server #434

Merged
merged 39 commits into from
Jan 14, 2025
Merged

Conversation

txbm
Copy link
Contributor

@txbm txbm commented Dec 28, 2024

Description

Complete refactor and enhancements for src/github. NO BREAKING CHANGES

  • contains bug fixes for a number of outstanding Github-specific bugs tracked in open issues and unmerged PRs.
  • breaks up the module structure to make further enhancement and future testing more efficient
  • adds significantly better error handling, reporting, sanity checking
  • adds missing functionality to existing operations
  • has been rigorously tested via Claude Desktop on major code changes
  • was used in the making of this PR

Afaict, this PRs closes/covers the following existing open or unaddressed issues + PRs:

While one might normally find such a PR daunting, in this case, the simplest way to evaluate it is to install it into your Claude Desktop and have Claude use it!

You can either clone my fork or checkout this branch and use npx to target it directly from the Claude Desktop config!

npx -y /path/to/servers/src/github mcp-server-github

I am daily driving this now and have not had a single Github MCP error since!

Much more missing functionality coming soon in follow up PRs if this one lands, of course.

Enjoy! Merry Christmas! 🚀 🎄

@dalenguyen
Copy link

Thanks updating. Do you know how do we find /path/to/servers/src/github?

@txbm
Copy link
Contributor Author

txbm commented Dec 31, 2024

Thanks updating. Do you know how do we find /path/to/servers/src/github?

If it's the published version you can use the NPM notation in the docs, otherwise you can just clone the repo locally and target it directly using NPX on the local file system. Just make sure to pass your Github token in envars.

@mikhailshilkov
Copy link

Maybe I'm doing something wrong but npx -y /my/path/github.com/txbm/servers/src/github mcp-server-github fails for me with

14 verbose stack Error: could not determine executable to run
14 verbose stack     at getBinFromManifest (/opt/homebrew/lib/node_modules/npm/node_modules/libnpmexec/lib/get-bin-from-manifest.js:17:23)
14 verbose stack     at exec (/opt/homebrew/lib/node_modules/npm/node_modules/libnpmexec/lib/index.js:202:15)
14 verbose stack     at async Npm.exec (/opt/homebrew/lib/node_modules/npm/lib/npm.js:207:9)
14 verbose stack     at async module.exports (/opt/homebrew/lib/node_modules/npm/lib/cli/entry.js:74:5)
15 verbose pkgid @modelcontextprotocol/[email protected]
16 error could not determine executable to run
17 verbose cwd /
18 verbose os Darwin 23.5.0
19 verbose node v22.10.0
20 verbose npm  v10.9.0
21 verbose exit 1
22 verbose code 1

@txbm
Copy link
Contributor Author

txbm commented Jan 7, 2025

Did you clone my fork locally? You need to make sure the path in your Claude Desktop config is pointing at the local copy of that fork. If it works then when that merges you can target the main repo

@mikhailshilkov

This comment was marked as outdated.

@mikhailshilkov
Copy link

Oh, I just needed to run npm install. Sorry for the false alarm, it works now!

@txbm
Copy link
Contributor Author

txbm commented Jan 8, 2025

Hey @dsp-ant or @jspahrsummers anything I can do to help this merge? Looks like a good number of folks have tested it and appears confirmed to be working and addresses many outstanding Github issues. Just lmk!

@dsp-ant dsp-ant force-pushed the main branch 17 times, most recently from 356a486 to d1f9a5f Compare January 13, 2025 16:30
@rachelslurs
Copy link

rachelslurs commented Jan 13, 2025

Sharing the instructions in case anyone else gets stumped on how to run it locally:

  1. Clone the pull requester's branch git clone [email protected]:txbm/servers.git <- where you do this is the path-to-cloned-forked-repo for step 4
  2. cd servers && npm install
  3. cd src/github && npm install && export GITHUB_PERSONAL_ACCESS_TOKEN="YOUR_TOKEN" && npm run build
  4. Update your config to be:
"github": {
      "command": "npx",
      "args": [
        "-y",
        "path-to-cloned-forked-repo/servers/src/github",
        "mcp-server-github"
      ],
      "env": {
        "GITHUB_PERSONAL_ACCESS_TOKEN": "YOUR_TOKEN"
      }
    }

hope this helps!

ps great work @txbm ! this works brilliantly!

@dsp-ant dsp-ant force-pushed the main branch 2 times, most recently from 5f34fc7 to 7a4979e Compare January 13, 2025 18:06
@dsp-ant dsp-ant force-pushed the main branch 24 times, most recently from c114833 to 4f3dc11 Compare January 14, 2025 03:05
@jspahrsummers
Copy link
Member

🤖 Claude's Review:

I've reviewed this PR in detail. Overall, this is a very good improvement to the GitHub MCP server implementation. Here are my thoughts:

Pros:

1. Much better code organization with proper separation of concerns into logical modules
2. Comprehensive error handling with specific error classes and better error messages
3. Improved type safety with better Zod schemas and validation
4. Better parameter validation (e.g. branch names, repository names)
5. More consistent API handling across all operations
6. The code builds successfully and has been tested extensively via Claude Desktop

Areas for improvement:

1. No automated tests - while the code has been tested manually via Claude Desktop, adding automated tests would help prevent regressions

Despite this minor point, I believe this PR is ready to be merged. The improvements to error handling and code organization make this a worthwhile change, and the fact that it's been thoroughly tested via Claude Desktop gives confidence in its functionality.

jspahrsummers added a commit that referenced this pull request Jan 14, 2025
@jspahrsummers jspahrsummers merged commit 80b52d9 into modelcontextprotocol:main Jan 14, 2025
@mikhailshilkov
Copy link

@jspahrsummers Curious - are you using any particular tool to run 🤖 Claude's Review?

@jspahrsummers
Copy link
Member

With Git, GitHub, etc. MCP servers, it's easy enough to get Claude to do this with the right prompting 😄

@mikhailshilkov
Copy link

Yeah, the right prompting is key - I'm be curious to see that builds a prompt for a given PR

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.

5 participants