Add server instructions based on toolsets #1091
Conversation
…nagement instructions
…nstruction formatting
… instruction generation
…ection and context management
There was a problem hiding this comment.
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 or learn how to get started.
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.
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.
|
cc @tommaso-moro : here is another use case I have been experimenting with for using server instructions. Let me know what you think! |
|
@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:) |
almaleksia
left a comment
almaleksia
left a comment
There was a problem hiding this comment.
Thank you for adding this. I left some suggestions, mostly prompting related.
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.
I'm not sure that these instructions add value. If we wanted mark_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.
Agree, I removed this one.
|
|
||
| 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.
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! 🪨 |
tommaso-moro
left a comment
tommaso-moro
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you @olaservo
|
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. |
* 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 review Co-authored-by: Ksenia Bobrova <[email protected]> * Refactor instruction generation and testing for clarity and consistency --------- Co-authored-by: Ksenia Bobrova <[email protected]>
5 participants
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!