Skip to content

Commit b6bf6cc

Browse files
SamMorrowDrumsSamMorrowDrums
committed
Address review feedback: enum size validation, mutation fix, tests
- Replace runtime size validation with compile-time enum type (Size with SizeSM=16, SizeLG=24) - Fix RegisterFunc mutation by making shallow copy of tool before modifying Icons - Add comprehensive tests for octicons package (URL, Icons, Size constants) - Add toolsets tests for ToolsetMetadata.Icons(), RegisterFunc mutation prevention, and existing icon preservation - Improve icon choices for better visual semantics: - actions: play → workflow (more specific to GitHub Actions) - secret_protection: key → shield-lock (better represents protection) - gists: code → logo-gist (dedicated gist icon exists)
1 parent c9d74c3 commit b6bf6cc

File tree

11 files changed

+251
-29
lines changed

11 files changed

+251
-29
lines changed

‎pkg/github/__toolsnaps__/assign_copilot_to_issue.snap‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,5 +26,21 @@
2626
}
2727
}
2828
},
29-
"name": "assign_copilot_to_issue"
29+
"name": "assign_copilot_to_issue",
30+
"icons": [
31+
{
32+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/copilot-16.svg",
33+
"mimeType": "image/svg+xml",
34+
"sizes": [
35+
"16x16"
36+
]
37+
},
38+
{
39+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/copilot-24.svg",
40+
"mimeType": "image/svg+xml",
41+
"sizes": [
42+
"24x24"
43+
]
44+
}
45+
]
3046
}

‎pkg/github/__toolsnaps__/fork_repository.snap‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,5 +24,21 @@
2424
}
2525
}
2626
},
27-
"name": "fork_repository"
27+
"name": "fork_repository",
28+
"icons": [
29+
{
30+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/repo-forked-16.svg",
31+
"mimeType": "image/svg+xml",
32+
"sizes": [
33+
"16x16"
34+
]
35+
},
36+
{
37+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/repo-forked-24.svg",
38+
"mimeType": "image/svg+xml",
39+
"sizes": [
40+
"24x24"
41+
]
42+
}
43+
]
2844
}

‎pkg/github/__toolsnaps__/merge_pull_request.snap‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,5 +42,21 @@
4242
}
4343
}
4444
},
45-
"name": "merge_pull_request"
45+
"name": "merge_pull_request",
46+
"icons": [
47+
{
48+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/git-merge-16.svg",
49+
"mimeType": "image/svg+xml",
50+
"sizes": [
51+
"16x16"
52+
]
53+
},
54+
{
55+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/git-merge-24.svg",
56+
"mimeType": "image/svg+xml",
57+
"sizes": [
58+
"24x24"
59+
]
60+
}
61+
]
4662
}

‎pkg/github/__toolsnaps__/request_copilot_review.snap‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,5 +25,21 @@
2525
}
2626
}
2727
},
28-
"name": "request_copilot_review"
28+
"name": "request_copilot_review",
29+
"icons": [
30+
{
31+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/copilot-16.svg",
32+
"mimeType": "image/svg+xml",
33+
"sizes": [
34+
"16x16"
35+
]
36+
},
37+
{
38+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/copilot-24.svg",
39+
"mimeType": "image/svg+xml",
40+
"sizes": [
41+
"24x24"
42+
]
43+
}
44+
]
2945
}

‎pkg/github/__toolsnaps__/star_repository.snap‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,5 +20,21 @@
2020
}
2121
}
2222
},
23-
"name": "star_repository"
23+
"name": "star_repository",
24+
"icons": [
25+
{
26+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/star-fill-16.svg",
27+
"mimeType": "image/svg+xml",
28+
"sizes": [
29+
"16x16"
30+
]
31+
},
32+
{
33+
"src": "https://raw.githubusercontent.com/primer/octicons/main/icons/star-fill-24.svg",
34+
"mimeType": "image/svg+xml",
35+
"sizes": [
36+
"24x24"
37+
]
38+
}
39+
]
2440
}

‎pkg/github/issues.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
ghErrors "github.com/github/github-mcp-server/pkg/errors"
1313
"github.com/github/github-mcp-server/pkg/inventory"
1414
"github.com/github/github-mcp-server/pkg/lockdown"
15+
"github.com/github/github-mcp-server/pkg/octicons"
1516
"github.com/github/github-mcp-server/pkg/sanitize"
1617
"github.com/github/github-mcp-server/pkg/translations"
1718
"github.com/github/github-mcp-server/pkg/utils"
@@ -1622,6 +1623,7 @@ func AssignCopilotToIssue(t translations.TranslationHelperFunc) inventory.Server
16221623
mcp.Tool{
16231624
Name: "assign_copilot_to_issue",
16241625
Description: t("TOOL_ASSIGN_COPILOT_TO_ISSUE_DESCRIPTION", description.String()),
1626+
Icons: octicons.Icons("copilot"),
16251627
Annotations: &mcp.ToolAnnotations{
16261628
Title: t("TOOL_ASSIGN_COPILOT_TO_ISSUE_USER_TITLE", "Assign Copilot to issue"),
16271629
ReadOnlyHint: false,

‎pkg/github/pullrequests.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
ghErrors "github.com/github/github-mcp-server/pkg/errors"
1717
"github.com/github/github-mcp-server/pkg/inventory"
1818
"github.com/github/github-mcp-server/pkg/lockdown"
19+
"github.com/github/github-mcp-server/pkg/octicons"
1920
"github.com/github/github-mcp-server/pkg/sanitize"
2021
"github.com/github/github-mcp-server/pkg/translations"
2122
"github.com/github/github-mcp-server/pkg/utils"
@@ -1086,6 +1087,7 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool
10861087
mcp.Tool{
10871088
Name: "merge_pull_request",
10881089
Description: t("TOOL_MERGE_PULL_REQUEST_DESCRIPTION", "Merge a pull request in a GitHub repository."),
1090+
Icons: octicons.Icons("git-merge"),
10891091
Annotations: &mcp.ToolAnnotations{
10901092
Title: t("TOOL_MERGE_PULL_REQUEST_USER_TITLE", "Merge pull request"),
10911093
ReadOnlyHint: false,
@@ -1855,6 +1857,7 @@ func RequestCopilotReview(t translations.TranslationHelperFunc) inventory.Server
18551857
mcp.Tool{
18561858
Name: "request_copilot_review",
18571859
Description: t("TOOL_REQUEST_COPILOT_REVIEW_DESCRIPTION", "Request a GitHub Copilot code review for a pull request. Use this for automated feedback on pull requests, usually before requesting a human reviewer."),
1860+
Icons: octicons.Icons("copilot"),
18581861
Annotations: &mcp.ToolAnnotations{
18591862
Title: t("TOOL_REQUEST_COPILOT_REVIEW_USER_TITLE", "Request Copilot review"),
18601863
ReadOnlyHint: false,

‎pkg/github/repositories.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
ghErrors "github.com/github/github-mcp-server/pkg/errors"
1313
"github.com/github/github-mcp-server/pkg/inventory"
14+
"github.com/github/github-mcp-server/pkg/octicons"
1415
"github.com/github/github-mcp-server/pkg/raw"
1516
"github.com/github/github-mcp-server/pkg/translations"
1617
"github.com/github/github-mcp-server/pkg/utils"
@@ -810,6 +811,7 @@ func ForkRepository(t translations.TranslationHelperFunc) inventory.ServerTool {
810811
mcp.Tool{
811812
Name: "fork_repository",
812813
Description: t("TOOL_FORK_REPOSITORY_DESCRIPTION", "Fork a GitHub repository to your account or specified organization"),
814+
Icons: octicons.Icons("repo-forked"),
813815
Annotations: &mcp.ToolAnnotations{
814816
Title: t("TOOL_FORK_REPOSITORY_USER_TITLE", "Fork repository"),
815817
ReadOnlyHint: false,
@@ -2096,6 +2098,7 @@ func StarRepository(t translations.TranslationHelperFunc) inventory.ServerTool {
20962098
mcp.Tool{
20972099
Name: "star_repository",
20982100
Description: t("TOOL_STAR_REPOSITORY_DESCRIPTION", "Star a GitHub repository"),
2101+
Icons: octicons.Icons("star-fill"),
20992102
Annotations: &mcp.ToolAnnotations{
21002103
Title: t("TOOL_STAR_REPOSITORY_USER_TITLE", "Star repository"),
21012104
ReadOnlyHint: false,

‎pkg/inventory/server_tool.go‎

Lines changed: 8 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ package inventory
33
import (
44
"context"
55
"encoding/json"
6-
"fmt"
76

7+
"github.com/github/github-mcp-server/pkg/octicons"
88
"github.com/modelcontextprotocol/go-sdk/mcp"
99
)
1010

@@ -33,29 +33,10 @@ type ToolsetMetadata struct {
3333
Icon string
3434
}
3535

36-
// OcticonURL returns the CDN URL for a GitHub Octicon SVG. Size should be 16 or 24.
37-
func OcticonURL(name string, size int) string {
38-
return fmt.Sprintf("https://raw.githubusercontent.com/primer/octicons/main/icons/%s-%d.svg", name, size)
39-
}
40-
4136
// Icons returns MCP Icon objects for this toolset, or nil if no icon is set.
4237
// Icons are provided in both 16x16 and 24x24 sizes.
4338
func (tm ToolsetMetadata) Icons() []mcp.Icon {
44-
if tm.Icon == "" {
45-
return nil
46-
}
47-
return []mcp.Icon{
48-
{
49-
Source: OcticonURL(tm.Icon, 16),
50-
MIMEType: "image/svg+xml",
51-
Sizes: []string{"16x16"},
52-
},
53-
{
54-
Source: OcticonURL(tm.Icon, 24),
55-
MIMEType: "image/svg+xml",
56-
Sizes: []string{"24x24"},
57-
},
58-
}
39+
return octicons.Icons(tm.Icon)
5940
}
6041

6142
// ServerTool represents an MCP tool with metadata and a handler generator function.
@@ -112,14 +93,17 @@ func (st *ServerTool) Handler(deps any) mcp.ToolHandler {
11293

11394
// RegisterFunc registers the tool with the server using the provided dependencies.
11495
// Icons are automatically applied from the toolset metadata if not already set.
96+
// A shallow copy of the tool is made to avoid mutating the original ServerTool.
11597
// Panics if the tool has no handler - all tools should have handlers.
11698
func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any) {
11799
handler := st.Handler(deps) // This will panic if HandlerFunc is nil
100+
// Make a shallow copy of the tool to avoid mutating the original
101+
toolCopy := st.Tool
118102
// Apply icons from toolset metadata if tool doesn't have icons set
119-
if len(st.Tool.Icons) == 0 {
120-
st.Tool.Icons = st.Toolset.Icons()
103+
if len(toolCopy.Icons) == 0 {
104+
toolCopy.Icons = st.Toolset.Icons()
121105
}
122-
s.AddTool(&st.Tool, handler)
106+
s.AddTool(&toolCopy, handler)
123107
}
124108

125109
// NewServerTool creates a ServerTool from a tool definition, toolset metadata, and a typed handler function.

‎pkg/octicons/octicons.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Package octicons provides helpers for working with GitHub Octicon icons.
2+
// See https://primer.style/foundations/icons for available icons.
3+
package octicons
4+
5+
import (
6+
"fmt"
7+
8+
"github.com/modelcontextprotocol/go-sdk/mcp"
9+
)
10+
11+
// Size represents the size of an Octicon icon.
12+
type Size int
13+
14+
const (
15+
// SizeSM is the small (16x16) icon size.
16+
SizeSM Size = 16
17+
// SizeLG is the large (24x24) icon size.
18+
SizeLG Size = 24
19+
)
20+
21+
// URL returns the CDN URL for a GitHub Octicon SVG.
22+
func URL(name string, size Size) string {
23+
return fmt.Sprintf("https://raw.githubusercontent.com/primer/octicons/main/icons/%s-%d.svg", name, size)
24+
}
25+
26+
// Icons returns MCP Icon objects for the given octicon name in both 16x16 and 24x24 sizes.
27+
// Use this to set custom icons on individual tools that should override their toolset's default icon.
28+
// The name should be the base octicon name without size suffix (e.g., "copilot" not "copilot-16").
29+
// See https://primer.style/foundations/icons for available icons.
30+
func Icons(name string) []mcp.Icon {
31+
if name == "" {
32+
return nil
33+
}
34+
return []mcp.Icon{
35+
{
36+
Source: URL(name, SizeSM),
37+
MIMEType: "image/svg+xml",
38+
Sizes: []string{"16x16"},
39+
},
40+
{
41+
Source: URL(name, SizeLG),
42+
MIMEType: "image/svg+xml",
43+
Sizes: []string{"24x24"},
44+
},
45+
}
46+
}

0 commit comments

Comments
 (0)