-
Notifications
You must be signed in to change notification settings - Fork 3k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
be881f8
Add instruction generation for enabled toolsets and corresponding tests
olaservo b78cb89
Refactor instruction generation to always include base and context ma…
olaservo bf3e525
Refactor base instruction for clarity and adjust context management i…
olaservo 0cd74fc
Simplify changes for now
olaservo 1690c70
Remove unused toolset instructions and simplify test cases for clarity
olaservo 1f500ab
Add test cases for issues, notifications, and discussions toolsets in…
olaservo fc3f378
Update base instruction and test expectations for clarity on tool sel…
olaservo 26951b5
Add support for disabling instructions via environment variable
olaservo e5645b9
Clarify PR review workflow instruction for consistency
olaservo c1386ff
Apply suggestions from code review
olaservo 914d487
Refactor instruction generation and testing for clarity and consistency
olaservo cb747e0
Merge branch 'main' into add-server-instructions
olaservo 9782f72
Merge branch 'main' into add-server-instructions
olaservo 858e230
Merge branch 'main' into add-server-instructions
almaleksia 15ab9c3
Merge branch 'main' into add-server-instructions
almaleksia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| package github | ||
|
|
||
| import ( | ||
| "os" | ||
| "slices" | ||
| "strings" | ||
| ) | ||
|
|
||
| // GenerateInstructions creates server instructions based on enabled toolsets | ||
| func GenerateInstructions(enabledToolsets []string) string { | ||
| // For testing - add a flag to disable instructions | ||
| if os.Getenv("DISABLE_INSTRUCTIONS") == "true" { | ||
| return "" // Baseline mode | ||
| } | ||
|
|
||
| var instructions []string | ||
|
|
||
| // Core instruction - always included if context toolset enabled | ||
| if slices.Contains(enabledToolsets, "context") { | ||
| instructions = append(instructions, "Always call 'get_me' first to understand current user permissions and context.") | ||
| } | ||
|
|
||
| // Individual toolset instructions | ||
| for _, toolset := range enabledToolsets { | ||
| if inst := getToolsetInstructions(toolset); inst != "" { | ||
| instructions = append(instructions, inst) | ||
| } | ||
| } | ||
|
|
||
| // Base instruction with context management | ||
| baseInstruction := `The GitHub MCP Server provides tools to interact with GitHub platform. | ||
|
|
||
| Tool selection guidance: | ||
| 1. 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. | ||
| 2. 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. 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.` | ||
|
|
||
| allInstructions := []string{baseInstruction} | ||
| allInstructions = append(allInstructions, instructions...) | ||
|
|
||
| return strings.Join(allInstructions, " ") | ||
| } | ||
|
|
||
| // getToolsetInstructions returns specific instructions for individual toolsets | ||
| func getToolsetInstructions(toolset string) string { | ||
| switch toolset { | ||
| case "pull_requests": | ||
| return "## Pull Requests\n\nPR review workflow: Always use 'create_pending_pull_request_review' → 'add_comment_to_pending_review' → 'submit_pending_pull_request_review' for complex reviews with line-specific comments." | ||
| case "issues": | ||
| return "## Issues\n\nCheck 'list_issue_types' first for organizations to use proper issue types. Use 'search_issues' before creating new issues to avoid duplicates. Always set 'state_reason' when closing issues." | ||
| case "discussions": | ||
| return "## Discussions\n\nUse 'list_discussion_categories' to understand available categories before creating discussions. Filter by category for better organization." | ||
| default: | ||
| return "" | ||
| } | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| package github | ||
|
|
||
| import ( | ||
| "os" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestGenerateInstructions(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| enabledToolsets []string | ||
| expectedEmpty bool | ||
| }{ | ||
| { | ||
| name: "empty toolsets", | ||
| enabledToolsets: []string{}, | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| name: "only context toolset", | ||
| enabledToolsets: []string{"context"}, | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| name: "pull requests toolset", | ||
| enabledToolsets: []string{"pull_requests"}, | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| name: "issues toolset", | ||
| enabledToolsets: []string{"issues"}, | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| name: "discussions toolset", | ||
| enabledToolsets: []string{"discussions"}, | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| name: "multiple toolsets (context + pull_requests)", | ||
| enabledToolsets: []string{"context", "pull_requests"}, | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| name: "multiple toolsets (issues + pull_requests)", | ||
| enabledToolsets: []string{"issues", "pull_requests"}, | ||
| expectedEmpty: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := GenerateInstructions(tt.enabledToolsets) | ||
|
|
||
| if tt.expectedEmpty { | ||
| if result != "" { | ||
| t.Errorf("Expected empty instructions but got: %s", result) | ||
| } | ||
| } else { | ||
| if result == "" { | ||
| t.Errorf("Expected non-empty instructions but got empty result") | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGenerateInstructionsWithDisableFlag(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| disableEnvValue string | ||
| enabledToolsets []string | ||
| expectedEmpty bool | ||
| }{ | ||
| { | ||
| name: "DISABLE_INSTRUCTIONS=true returns empty", | ||
| disableEnvValue: "true", | ||
| enabledToolsets: []string{"context", "issues", "pull_requests"}, | ||
| expectedEmpty: true, | ||
| }, | ||
| { | ||
| name: "DISABLE_INSTRUCTIONS=false returns normal instructions", | ||
| disableEnvValue: "false", | ||
| enabledToolsets: []string{"context"}, | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| name: "DISABLE_INSTRUCTIONS unset returns normal instructions", | ||
| disableEnvValue: "", | ||
| enabledToolsets: []string{"issues"}, | ||
| expectedEmpty: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Save original env value | ||
| originalValue := os.Getenv("DISABLE_INSTRUCTIONS") | ||
| defer func() { | ||
| if originalValue == "" { | ||
| os.Unsetenv("DISABLE_INSTRUCTIONS") | ||
| } else { | ||
| os.Setenv("DISABLE_INSTRUCTIONS", originalValue) | ||
| } | ||
| }() | ||
|
|
||
| // Set test env value | ||
| if tt.disableEnvValue == "" { | ||
| os.Unsetenv("DISABLE_INSTRUCTIONS") | ||
| } else { | ||
| os.Setenv("DISABLE_INSTRUCTIONS", tt.disableEnvValue) | ||
| } | ||
|
|
||
| result := GenerateInstructions(tt.enabledToolsets) | ||
|
|
||
| if tt.expectedEmpty { | ||
| if result != "" { | ||
| t.Errorf("Expected empty instructions but got: %s", result) | ||
| } | ||
| } else { | ||
| if result == "" { | ||
| t.Errorf("Expected non-empty instructions but got empty result") | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGetToolsetInstructions(t *testing.T) { | ||
| tests := []struct { | ||
| toolset string | ||
| expectedEmpty bool | ||
| }{ | ||
| { | ||
| toolset: "pull_requests", | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| toolset: "issues", | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| toolset: "discussions", | ||
| expectedEmpty: false, | ||
| }, | ||
| { | ||
| toolset: "nonexistent", | ||
| expectedEmpty: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.toolset, func(t *testing.T) { | ||
| result := getToolsetInstructions(tt.toolset) | ||
| if tt.expectedEmpty { | ||
| if result != "" { | ||
| t.Errorf("Expected empty result for toolset '%s', but got: %s", tt.toolset, result) | ||
| } | ||
| } else { | ||
| if result == "" { | ||
| t.Errorf("Expected non-empty result for toolset '%s', but got empty", tt.toolset) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.