-
Notifications
You must be signed in to change notification settings - Fork 636
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 is powered by AI, so mistakes are possible. Review output carefully before use.
4141d2f
to
ed61afc
Compare
pkg/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 use https://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. |
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 using all
instead of everything
?
As it's very minor, feel free to ignore it 😄
ed61afc
to
3f38134
Compare
3f38134
to
b0bcb21
Compare
This is so 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
to
9a56c25
Compare
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.
Looking great! 🚀
8d56fe2
to
6ecee2f
Compare
What 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. |
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
to
8aba085
Compare
"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
to
1dea445
Compare
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
--features
flag. This allows you to control which GitHub API capabilities are available to your AI tools.Available Features
The following feature groups are available:
repos
issues
search
pull_requests
code_security
experiments
everything
Specifying 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-server
The environment variable
GITHUB_FEATURES
takes precedence over the command line argument if both are provided.Default Enabled Features
By default, the following features are enabled:
repos
issues
pull_requests
search
Using With Docker
When using Docker, you can pass the features as environment variables:
The "everything" Feature
The special feature
everything
can be provided to enable all available features regardless of their individual settings:Or using the environment variable:
GITHUB_FEATURES="everything" ./github-mcp-server