- Notifications
You must be signed in to change notification settings - Fork2.7k
Add server instructions based on toolsets#1091
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
…nagement instructions
…nstruction formatting
… instruction generation
…ection and context management
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 adds server instructions based on enabled toolsets to the GitHub MCP Server. The implementation generates context-aware instructions that help clients use the GitHub API tools more effectively based on which functionality is enabled.
- Introduces a
GenerateInstructionsfunction that creates tailored instructions based on enabled toolsets - Adds comprehensive test coverage for instruction generation with different toolset combinations
- Integrates the instruction system into the server initialization process
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/github/instructions.go | Implements the core instruction generation logic with toolset-specific guidance |
| pkg/github/instructions_test.go | Provides comprehensive test coverage for instruction generation scenarios |
| internal/ghmcp/server.go | Integrates instruction generation into the MCP server initialization |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
pkg/github/instructions.go Outdated
| } | ||
| // Base instruction with context management | ||
| baseInstruction := "The GitHub MCP Server provides GitHub API tools. Tool selection guidance: Use 'list_*' tools for broad, simple retrieval and pagination of all items of a type (e.g., all issues, all PRs, all branches) with basic filtering. Use 'search_*' tools for targeted queries with specific criteria, keywords, or complex filters (e.g., issues with certain text, PRs by author, code containing functions). Context management: 1) GitHub API responses can overflow context windows, 2) Process large datasets in batches of 5-10 items, 3) For summarization tasks, fetch minimal data first, then drill down into specifics." |
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.
This long string literal is difficult to read and maintain. Consider breaking it into smaller constants or using a multi-line string with proper formatting to improve readability.
Copilot uses AI. Check for mistakes.
Uh oh!
There was an error while loading.Please reload this page.
cc@tommaso-moro : here is another use case I have been experimenting with for using server instructions. Let me know what you think! |
tommaso-moro commentedSep 19, 2025 • 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.
@olaservo thank you for your work on this! This PR is in our radar and as you know we are discussing server instructions internally, so I will be in touch when we have updates:) |
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.
Thank you for adding this. I left some suggestions, mostly prompting related.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
pkg/github/instructions.go Outdated
| case "notifications": | ||
| return "Notifications: Filter by 'participating' for issues/PRs you're involved in. Use 'mark_all_notifications_read' with repository filters to avoid marking unrelated notifications." |
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'm not sure that these instructions add value. If we wantedmark_all_notifications_read to be always called with repo argument we'd make this parameter required.
Regarding filtering - these instructions already included in the tool description.
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.
Agree, I removed this one.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Ksenia Bobrova <almaleksia@github.com>
| Context management: | ||
| 1. Use pagination whenever possible with batches of 5-10 items. | ||
| 2. Use minimal_output parameter set to true if the full information is not needed to accomplish a task.` |
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.
Very good catch, thank you.
The llm cant be really confident in knowing what is contained in the minimal vs the verbose output. But since we have not established a standardized format for all offenders(big tool replies) I think this is good.
Thanks so much for this@olaservo you rock! 🪨 |
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! Thank you@olaservo
tonytrg commentedSep 24, 2025 • 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.
One thought I had - not blocking this review process though. Should we add a cli flag/env var config option I think that could be a nice feature for agents with specifc circumstances to customize their tool usage behaviour. |
0a1d6db intogithub:mainUh oh!
There was an error while loading.Please reload this page.
* Add instruction generation for enabled toolsets and corresponding tests* Refactor instruction generation to always include base and context management instructions* Refactor base instruction for clarity and adjust context management instruction formatting* Simplify changes for now* Remove unused toolset instructions and simplify test cases for clarity* Add test cases for issues, notifications, and discussions toolsets in instruction generation* Update base instruction and test expectations for clarity on tool selection and context management* Add support for disabling instructions via environment variable* Clarify PR review workflow instruction for consistency* Apply suggestions from code reviewCo-authored-by: Ksenia Bobrova <almaleksia@github.com>* Refactor instruction generation and testing for clarity and consistency---------Co-authored-by: Ksenia Bobrova <almaleksia@github.com>
Uh oh!
There was an error while loading.Please reload this page.
Hi, I am working on a post for the official MCP blog that focuses on server instructions (since clients have recently started to support this feature).
So far, I did some simple controlled evaluations using these changes locally, which you can find more details on here:https://github.com/olaservo/mcp-server-instructions-demo
Here is the PR for my blog post:modelcontextprotocol/modelcontextprotocol#1482
I haven't methodically tested all of these yet, but can do a similar evaluation and expand my tests to more models and clients, if we think this approach is promising.
Thanks!