- Notifications
You must be signed in to change notification settings - Fork897
feat: partition tools by product/feature#188
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this 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 partitions the GitHub MCP Server tools by product area using feature flags, and adds the necessary configuration options and tests.
- Introduces a new "features" module to manage feature sets and enable/disable specific tools.
- Updates the server initialization to conditionally add tools based on feature flags.
- Adds tests and updates documentation to reflect the new configurable features.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/server_test.go | Added tests (e.g., Test_ListAvailableFeatures) for verifying the feature listing tool. |
| pkg/github/server.go | Modified server creation to conditionally add tools based on enabled feature flags. |
| pkg/features/features_test.go | Added unit tests to validate FeatureSet behavior. |
| pkg/features/features.go | Introduced FeatureSet implementation for feature management. |
| cmd/github-mcp-server/main.go | Updated CLI and environment variable parsing to initialize features. |
| README.md | Expanded documentation on the new features configuration and usage. |
Comments suppressed due to low confidence (2)
cmd/github-mcp-server/main.go:91
- [nitpick] Consider renaming the variable 'envFeats' to 'envFeatures' to improve clarity.
if envFeats := os.Getenv("GITHUB_FEATURES"); envFeats != "" {pkg/features/features.go:63
- [nitpick] Consider wrapping the feature name in quotes within the error message (e.g. "feature '%s' does not exist") for improved clarity.
return fmt.Errorf("feature %s does not exist", name)pkg/github/server.go Outdated
| func(_ context.Context, _ mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| // We need to convert the FeatureSet back to a map for JSON serialization | ||
| featureMap := make(map[string]bool) | ||
| for name := range featureSet.Features { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If a deterministic order of features is required for clients consuming the JSON output, consider sorting the feature keys before JSON marshaling.
Copilot uses AI. Check for mistakes.
4141d2f toed61afcComparepkg/features/features.go Outdated
| } | ||
| } | ||
| func (fs *FeatureSet) AddFeature(name string, description string, enabled bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: if you ever need to usehttps://pkg.go.dev/io/fs in here one or the other will need to be renamed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great point!
I might consider calling them "toolsets" instead of features, just to be a bit more explicit. |
@toby toolsets does make sense, I hesitate a little because resources and prompts are also part of it, so I was sort of avoiding the word tool, but perhaps we just run with it? Definitely not committed to any naming. |
juruen left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me!
Just one super minor question: what would you think about usingall instead ofeverything?
As it's very minor, feel free to ignore it 😄
ed61afc to3f38134Compare3f38134 tob0bcb21CompareThis isso great!! |
Aside from the linter, do you plan on merging this soon? |
Yeah, I just have to finish a couple of bits. Mostly the auto tool updates. Just keep pushing the latest. |
0ee52f5 to9a56c25CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looking great! 🚀
8d56fe2 to6ecee2fCompareWhat do you think about these options?
|
I think those suggestions get us closer to a usable interface for sure, will riff on them on Monday I hope, see if we end up with something mergeable. Feels close. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
README.md Outdated
| - `issues` | ||
| - `pull_requests` | ||
| - `search` | ||
| - `context-_ools` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Typo
pkg/toolsets/toolsets.go Outdated
| readOnly bool | ||
| writeTools []server.ServerTool | ||
| readTools []server.ServerTool | ||
| resourceTemplates []ResourceTemplate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Feedback
Was a bit confusing to me that toolsets also manage resource templates. I don't see a relationship between them. Might be indicative that the abstraction is trying to do too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll leave it separate then, initially the idea was that all features pertaining to a product area (not just tools) would be adjusted - and the naming reflected that - but I merged it into toolsets.
For now I'll leave it separate, and it can be revisited.
45f55fc to8aba085Compare"none" also might be useful? As in, "I do not currently want any of these tools enabled, but nor do I want to remove my config entirely from settings" (and since .vscode/mcp.json is, well, json, commenting out the entire block seems like a non-starter). |
Interesting idea@ismith
Some other software might not support that, but I'm tempted to say we once this merges we could track the |
8aba085 to1dea445Compareff3036d intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Partitioned Features
This PR proposes a method for partitioning features by product area, and provides ENV and CLI flag options to configure it on server startup.
Features Configuration
The GitHub MCP Server supports enabling or disabling specific groups of functionalities via the
--featuresflag. This allows you to control which GitHub API capabilities are available to your AI tools.Available Features
The following feature groups are available:
reposissuessearchpull_requestscode_securityexperimentseverythingSpecifying Features
You can enable specific features in two ways:
Using Command Line Argument:
Using Environment Variable:
GITHUB_FEATURES="repos,issues,pull_requests,code_security" ./github-mcp-serverThe environment variable
GITHUB_FEATUREStakes precedence over the command line argument if both are provided.Default Enabled Features
By default, the following features are enabled:
reposissuespull_requestssearchUsing With Docker
When using Docker, you can pass the features as environment variables:
The "everything" Feature
The special feature
everythingcan be provided to enable all available features regardless of their individual settings:Or using the environment variable:
GITHUB_FEATURES="everything" ./github-mcp-server