diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 975df2a633..b889a51c3a 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -5,7 +5,7 @@ This is the **GitHub MCP Server**, a Model Context Protocol (MCP) server that connects AI tools to GitHub's platform. It enables AI agents to manage repositories, issues, pull requests, workflows, and more through natural language. **Key Details:** -- **Language:** Go 1.24+ (~38k lines of code) +- **Language:** Go (minimum version specified in `go.mod`) - **Type:** MCP server application with CLI interface - **Primary Package:** github-mcp-server (stdio MCP server - **this is the main focus**) - **Secondary Package:** mcpcurl (testing utility - don't break it, but not the priority) @@ -92,7 +92,7 @@ go test ./pkg/github -run TestGetMe ### Key Configuration Files -- **go.mod / go.sum:** Go module dependencies (Go 1.24.0+) +- **go.mod / go.sum:** Go module dependencies and minimum supported Go version - **.golangci.yml:** Linter configuration (v2 format, ~15 linters enabled) - **Dockerfile:** Multi-stage build (golang:1.25.8-alpine → distroless) - **server.json:** MCP server metadata for registry @@ -112,10 +112,10 @@ go test ./pkg/github -run TestGetMe ## GitHub Workflows (CI/CD) -All workflows run on push/PR unless noted. Located in `.github/workflows/`: +Workflow triggers vary; consult the files in `.github/workflows/`: 1. **go.yml** - Build and test on ubuntu/windows/macos. Runs `script/test` and builds binary -2. **lint.yml** - Runs golangci-lint-action v2.5 (GitHub Action) with actions/setup-go stable +2. **lint.yml** - Runs golangci-lint-action v9 with Go 1.25 and golangci-lint v2.9.0, matching the version enforced by `script/lint` 3. **docs-check.yml** - Verifies README.md is up-to-date by running generate-docs and checking git diff 4. **code-scanning.yml** - CodeQL security analysis for Go and GitHub Actions 5. **license-check.yml** - Runs `script/licenses-check` to validate compliance @@ -123,7 +123,7 @@ All workflows run on push/PR unless noted. Located in `.github/workflows/`: 7. **goreleaser.yml** - Creates releases (main branch only) 8. **registry-releaser.yml** - Updates MCP registry -**All of these must pass for PR merge.** If docs-check fails, run `script/generate-docs` and commit changes. +**Applicable PR checks must pass for merge.** Publishing workflows run on their configured release events, not on every PR. CodeQL may be intentionally skipped on forks. If docs-check fails, run `script/generate-docs` and commit changes. ## Testing Guidelines diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 6119ee9f0f..ab08247676 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -23,4 +23,4 @@ jobs: uses: golangci/golangci-lint-action@v9 with: # sync with script/lint - version: v2.9 + version: v2.9.0 diff --git a/docs/installation-guides/README.md b/docs/installation-guides/README.md index 46581aa77e..ef5a043bb4 100644 --- a/docs/installation-guides/README.md +++ b/docs/installation-guides/README.md @@ -65,8 +65,8 @@ The GitHub MCP Server can be installed using several methods. **Docker is the mo ### 🔨 Build from Source (Advanced Users) - **Pros**: Latest features, full customization, no external dependencies - **Cons**: Requires Go development environment, more complex setup -- **Prerequisites**: [Go 1.24+](https://go.dev/doc/install) -- **Build command**: `go build -o github-mcp-server cmd/github-mcp-server/main.go` +- **Prerequisites**: [Go](https://go.dev/doc/install), at least the version specified in [`go.mod`](../../go.mod) +- **Build command**: `go build -o github-mcp-server ./cmd/github-mcp-server` - **Best for**: Developers who want the latest features or need custom modifications ### Important Notes on the GitHub MCP Server @@ -82,7 +82,7 @@ All installations with Personal Access Tokens (PAT) require: Optional (depending on installation method): - **Docker** (for Docker-based installations): [Download Docker](https://www.docker.com/) -- **Go 1.24+** (for building from source): [Install Go](https://go.dev/doc/install) +- **Go** (for building from source, at least the version in [`go.mod`](../../go.mod)): [Install Go](https://go.dev/doc/install) ## Security Best Practices @@ -109,4 +109,3 @@ After installation, you may want to explore: - **Toolsets**: Enable/disable specific GitHub API capabilities - **Read-Only Mode**: Restrict to read-only operations - **Lockdown Mode**: Hide public issue details created by users without push access - diff --git a/pkg/context/mcp_info.go b/pkg/context/mcp_info.go index af474b13a0..55028f7487 100644 --- a/pkg/context/mcp_info.go +++ b/pkg/context/mcp_info.go @@ -22,11 +22,22 @@ type MCPMethodInfo struct { ItemName string // RawArguments contains the unmaterialized tool arguments for tools/call requests. RawArguments json.RawMessage + // Deprecated: Owner is retained for source compatibility and is not populated + // by the parse middleware. Use DecodeArguments for call-specific values. + Owner string + // Deprecated: Repo is retained for source compatibility and is not populated + // by the parse middleware. Use DecodeArguments for call-specific values. + Repo string + // Deprecated: Arguments is retained for source compatibility and is not + // populated by the parse middleware. Use DecodeArguments instead. + Arguments map[string]any } // DecodeArguments materializes tool arguments when request middleware needs // call-specific values. Invalid argument shapes are returned to the caller so // the request can continue to the tool handler's normal validation path. +// Each call decodes RawArguments anew; decoded maps are not cached and the +// deprecated fields are neither read nor populated. func (info *MCPMethodInfo) DecodeArguments() (map[string]any, error) { if len(info.RawArguments) == 0 { return nil, nil diff --git a/pkg/context/mcp_info_test.go b/pkg/context/mcp_info_test.go new file mode 100644 index 0000000000..cd85fd61a1 --- /dev/null +++ b/pkg/context/mcp_info_test.go @@ -0,0 +1,137 @@ +package context + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMCPMethodInfoDecodeArguments(t *testing.T) { + tests := []struct { + name string + raw string + want map[string]any + wantErr bool + }{ + {name: "missing"}, + {name: "null", raw: `null`}, + {name: "empty object", raw: `{}`, want: map[string]any{}}, + {name: "owner only", raw: `{"owner":"github"}`, want: map[string]any{"owner": "github"}}, + {name: "repo only", raw: `{"repo":"server"}`, want: map[string]any{"repo": "server"}}, + { + name: "exact and differently cased keys remain distinct", + raw: `{"owner":"github","Owner":"other","repo":"server","REPO":"other"}`, + want: map[string]any{"owner": "github", "Owner": "other", "repo": "server", "REPO": "other"}, + }, + { + name: "differently cased keys do not introduce lowercase keys", + raw: `{"Owner":"other","REPO":"other"}`, + want: map[string]any{"Owner": "other", "REPO": "other"}, + }, + { + name: "escaped keys and values", + raw: `{"ow\u006eer":"git\u0068ub","re\u0070o":"server","\u004fwner":null}`, + want: map[string]any{"owner": "github", "repo": "server", "Owner": nil}, + }, + { + name: "last duplicate wins including null", + raw: `{"owner":123,"owner":"github","repo":"server","repo":null}`, + want: map[string]any{"owner": "github", "repo": nil}, + }, + { + name: "escaped duplicate replaces exact key", + raw: `{"owner":"github","ow\u006eer":null,"repo":[],"repo":"server"}`, + want: map[string]any{"owner": nil, "repo": "server"}, + }, + { + name: "duplicate becomes object", + raw: `{"owner":"github","owner":{},"repo":"server"}`, + want: map[string]any{"owner": map[string]any{}, "repo": "server"}, + }, + { + name: "wrongly typed owner does not discard repo", + raw: `{"owner":123,"repo":"server"}`, + want: map[string]any{"owner": float64(123), "repo": "server"}, + }, + { + name: "wrongly typed repo does not discard owner", + raw: `{"owner":"github","repo":false}`, + want: map[string]any{"owner": "github", "repo": false}, + }, + { + name: "nested policy arguments are retained", + raw: `{"files":[{"path":".github/workflows/ci.yml"}]}`, + want: map[string]any{"files": []any{map[string]any{"path": ".github/workflows/ci.yml"}}}, + }, + {name: "array shape", raw: `[]`, wantErr: true}, + {name: "string shape", raw: `"not an object"`, wantErr: true}, + {name: "number shape", raw: `123`, wantErr: true}, + {name: "boolean shape", raw: `true`, wantErr: true}, + {name: "malformed JSON", raw: `{"owner":"github","repo":}`, wantErr: true}, + {name: "trailing JSON", raw: `{"owner":"github"} {}`, wantErr: true}, + {name: "unrepresentable number", raw: `{"owner":"github","extra":1e1000}`, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + info := MCPMethodInfo{RawArguments: json.RawMessage(tt.raw)} + arguments, err := info.DecodeArguments() + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, arguments, "a failed decode must not expose a partial map") + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, arguments) + } + assert.Equal(t, MCPMethodInfo{RawArguments: json.RawMessage(tt.raw)}, info, + "decoding must not mutate raw arguments or populate legacy fields") + }) + } +} + +func TestMCPMethodInfoDecodeArgumentsReturnsIndependentMaps(t *testing.T) { + const raw = `{"owner":"github","files":[{"path":"README.md"}]}` + info := MCPMethodInfo{RawArguments: json.RawMessage(raw)} + + first, err := info.DecodeArguments() + require.NoError(t, err) + first["owner"] = "changed" + files, ok := first["files"].([]any) + require.True(t, ok) + file, ok := files[0].(map[string]any) + require.True(t, ok) + file["path"] = "changed" + + second, err := info.DecodeArguments() + require.NoError(t, err) + assert.Equal(t, map[string]any{ + "owner": "github", + "files": []any{map[string]any{"path": "README.md"}}, + }, second) + assert.Equal(t, MCPMethodInfo{RawArguments: json.RawMessage(raw)}, info) +} + +func TestMCPMethodInfoDecodeArgumentsIgnoresLegacyFields(t *testing.T) { + for _, raw := range []string{"", `{"owner":"current"}`} { + t.Run(raw, func(t *testing.T) { + info := MCPMethodInfo{ + RawArguments: json.RawMessage(raw), + Owner: "legacy-owner", + Repo: "legacy-repo", + Arguments: map[string]any{"owner": "legacy-argument"}, + } + arguments, err := info.DecodeArguments() + require.NoError(t, err) + if raw == "" { + assert.Nil(t, arguments) + } else { + assert.Equal(t, map[string]any{"owner": "current"}, arguments) + } + assert.Equal(t, "legacy-owner", info.Owner) + assert.Equal(t, "legacy-repo", info.Repo) + assert.Equal(t, map[string]any{"owner": "legacy-argument"}, info.Arguments) + }) + } +} diff --git a/pkg/http/middleware/mcp_parse_test.go b/pkg/http/middleware/mcp_parse_test.go index e067f7808a..e98a5661b7 100644 --- a/pkg/http/middleware/mcp_parse_test.go +++ b/pkg/http/middleware/mcp_parse_test.go @@ -54,6 +54,20 @@ func TestWithMCPParse(t *testing.T) { body: "not valid json", expectInfo: false, }, + { + name: "malformed arguments are skipped", + method: http.MethodPost, + path: "/mcp", + body: `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"test_tool","arguments":{"owner":"github","repo":}}}`, + expectInfo: false, + }, + { + name: "trailing JSON is skipped", + method: http.MethodPost, + path: "/mcp", + body: `{"jsonrpc":"2.0","method":"tools/call","params":{"arguments":{"owner":"github"}}} {}`, + expectInfo: false, + }, { name: "non-JSON-RPC 2.0 is skipped", method: http.MethodPost, @@ -86,7 +100,7 @@ func TestWithMCPParse(t *testing.T) { expectedItem: "get_file_contents", }, { - name: "tools/call parses owner and repo from arguments", + name: "tools/call retains arguments for explicit decoding", method: http.MethodPost, path: "/mcp", body: `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"get_file_contents","arguments":{"owner":"github","repo":"github-mcp-server","path":"README.md"}}}`, @@ -97,7 +111,16 @@ func TestWithMCPParse(t *testing.T) { expectedArgs: map[string]any{"owner": "github", "repo": "github-mcp-server", "path": "README.md"}, }, { - name: "tools/call with invalid arguments JSON continues without args", + name: "tools/call retains large extra arguments fields", + method: http.MethodPost, + path: "/mcp", + body: `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"get_file_contents","arguments":{"owner":"github","repo":"github-mcp-server","path":"README.md","extra":"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"}}}`, + expectInfo: true, + expectedMethod: "tools/call", + expectedItem: "get_file_contents", + }, + { + name: "tools/call with invalid argument shape continues with raw args", method: http.MethodPost, path: "/mcp", body: `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"get_file_contents","arguments":"not an object"}}`, @@ -107,6 +130,17 @@ func TestWithMCPParse(t *testing.T) { expectedRaw: `"not an object"`, expectArgsError: true, }, + { + name: "tools/call retains wrongly typed owner for explicit decoding", + method: http.MethodPost, + path: "/mcp", + body: `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"get_file_contents","arguments":{"owner":123,"repo":"github-mcp-server"}}}`, + expectInfo: true, + expectedMethod: "tools/call", + expectedItem: "get_file_contents", + expectedRaw: `{"owner":123,"repo":"github-mcp-server"}`, + expectedArgs: map[string]any{"owner": float64(123), "repo": "github-mcp-server"}, + }, { name: "prompts/get parses name", method: http.MethodPost, @@ -141,8 +175,12 @@ func TestWithMCPParse(t *testing.T) { var infoCaptured bool // Create a handler that captures the MCPMethodInfo from context - nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { capturedInfo, infoCaptured = ghcontext.MCPMethod(r.Context()) + restoredBody, err := io.ReadAll(r.Body) + require.NoError(t, err) + assert.Equal(t, tt.body, string(restoredBody)) + w.WriteHeader(http.StatusNoContent) }) middleware := WithMCPParse() @@ -153,21 +191,27 @@ func TestWithMCPParse(t *testing.T) { handler.ServeHTTP(rr, req) + assert.Equal(t, http.StatusNoContent, rr.Code, "parsing must not block downstream handlers") if tt.expectInfo { require.True(t, infoCaptured, "MCPMethodInfo should be present in context") require.NotNil(t, capturedInfo) assert.Equal(t, tt.expectedMethod, capturedInfo.Method) assert.Equal(t, tt.expectedItem, capturedInfo.ItemName) + assert.Equal(t, &ghcontext.MCPMethodInfo{ + Method: tt.expectedMethod, + ItemName: tt.expectedItem, + RawArguments: capturedInfo.RawArguments, + }, capturedInfo, "legacy fields must remain unpopulated") if tt.expectedRaw != "" { - assert.JSONEq(t, tt.expectedRaw, string(capturedInfo.RawArguments)) - } - decodedArgs, err := capturedInfo.DecodeArguments() - if tt.expectArgsError { - assert.Error(t, err) - } else { - require.NoError(t, err) + assert.Equal(t, tt.expectedRaw, string(capturedInfo.RawArguments)) } - if tt.expectedArgs != nil { + if tt.expectedArgs != nil || tt.expectArgsError { + decodedArgs, err := capturedInfo.DecodeArguments() + if tt.expectArgsError { + assert.Error(t, err) + } else { + require.NoError(t, err) + } assert.Equal(t, tt.expectedArgs, decodedArgs) } } else { @@ -177,6 +221,82 @@ func TestWithMCPParse(t *testing.T) { } } +func TestWithMCPParse_ArgumentTypes(t *testing.T) { + tests := []struct { + name string + arguments string + owner string + repo string + expectArgsError bool + }{ + {name: "empty object", arguments: `{}`}, + {name: "null arguments", arguments: `null`}, + {name: "array arguments", arguments: `[]`, expectArgsError: true}, + {name: "numeric arguments", arguments: `123`, expectArgsError: true}, + {name: "string arguments", arguments: `"not an object"`, expectArgsError: true}, + {name: "boolean arguments", arguments: `true`, expectArgsError: true}, + {name: "owner only", arguments: `{"owner":"github"}`, owner: "github"}, + {name: "repo only", arguments: `{"repo":"server"}`, repo: "server"}, + {name: "object owner", arguments: `{"owner":{},"repo":"server"}`, repo: "server"}, + {name: "array owner", arguments: `{"owner":[],"repo":"server"}`, repo: "server"}, + {name: "boolean owner", arguments: `{"owner":true,"repo":"server"}`, repo: "server"}, + {name: "null owner", arguments: `{"owner":null,"repo":"server"}`, repo: "server"}, + {name: "numeric repo", arguments: `{"owner":"github","repo":123}`, owner: "github"}, + {name: "object repo", arguments: `{"owner":"github","repo":{}}`, owner: "github"}, + {name: "array repo", arguments: `{"owner":"github","repo":[]}`, owner: "github"}, + {name: "boolean repo", arguments: `{"owner":"github","repo":false}`, owner: "github"}, + {name: "null repo", arguments: `{"owner":"github","repo":null}`, owner: "github"}, + {name: "both invalid", arguments: `{"owner":[],"repo":{}}`}, + {name: "last duplicate wins", arguments: `{"owner":123,"owner":"github","repo":"server","repo":null}`, owner: "github"}, + { + name: "large nested extra arguments", + arguments: `{"extra":[` + + strings.Repeat(`{"owner":"ignored","repo":"ignored","content":"`+strings.Repeat("x", 1024)+`"},`, 127) + + `{}],"owner":"github","repo":"server"}`, + owner: "github", + repo: "server", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body := `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"test_tool","arguments":` + tt.arguments + `}}` + var nextCalled bool + next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + nextCalled = true + info, ok := ghcontext.MCPMethod(r.Context()) + require.True(t, ok) + require.NotNil(t, info) + assert.Equal(t, "tools/call", info.Method) + assert.Equal(t, "test_tool", info.ItemName) + assert.Equal(t, tt.arguments, string(info.RawArguments)) + assert.Equal(t, &ghcontext.MCPMethodInfo{ + Method: "tools/call", + ItemName: "test_tool", + RawArguments: info.RawArguments, + }, info, "legacy fields must remain unpopulated") + arguments, err := info.DecodeArguments() + if tt.expectArgsError { + assert.Error(t, err) + } else { + require.NoError(t, err) + } + owner, _ := arguments["owner"].(string) + repo, _ := arguments["repo"].(string) + assert.Equal(t, tt.owner, owner) + assert.Equal(t, tt.repo, repo) + + restoredBody, err := io.ReadAll(r.Body) + require.NoError(t, err) + assert.Equal(t, body, string(restoredBody)) + }) + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + WithMCPParse()(next).ServeHTTP(httptest.NewRecorder(), req) + assert.True(t, nextCalled) + }) + } +} + func TestWithMCPParseRetainsLargeArgumentsWithoutMaterializingThem(t *testing.T) { nested := map[string]any{ "items": []any{ @@ -186,17 +306,96 @@ func TestWithMCPParseRetainsLargeArgumentsWithoutMaterializingThem(t *testing.T) } rawArguments, err := json.Marshal(nested) require.NoError(t, err) - body := `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"test_tool","arguments":` + string(rawArguments) + `}}` - - var capturedInfo *ghcontext.MCPMethodInfo - next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - capturedInfo, _ = ghcontext.MCPMethod(r.Context()) - }) - request := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) - WithMCPParse()(next).ServeHTTP(httptest.NewRecorder(), request) + tests := []struct { + name string + raw string + }{ + {name: "nested arrays and objects", raw: string(rawArguments)}, + { + name: "large nested extra arguments", + raw: `{"extra":[` + + strings.Repeat(`{"owner":"ignored","repo":"ignored","content":"`+strings.Repeat("x", 1024)+`"},`, 127) + + `{}],"owner":"github","repo":"server"}`, + }, + {name: "unrepresentable number remains raw", raw: `{"owner":"github","extra":1e1000}`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body := `{"jsonrpc":"2.0","method":"tools/call","params":{"name":"test_tool","arguments":` + tt.raw + `}}` + var capturedInfo *ghcontext.MCPMethodInfo + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedInfo, _ = ghcontext.MCPMethod(r.Context()) + restoredBody, err := io.ReadAll(r.Body) + require.NoError(t, err) + assert.Equal(t, body, string(restoredBody)) + w.WriteHeader(http.StatusNoContent) + }) + request := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + response := httptest.NewRecorder() + WithMCPParse()(next).ServeHTTP(response, request) + + assert.Equal(t, http.StatusNoContent, response.Code) + require.NotNil(t, capturedInfo) + assert.Equal(t, &ghcontext.MCPMethodInfo{ + Method: "tools/call", + ItemName: "test_tool", + RawArguments: json.RawMessage(tt.raw), + }, capturedInfo, "only raw metadata should be populated") + }) + } +} - require.NotNil(t, capturedInfo) - assert.Equal(t, json.RawMessage(rawArguments), capturedInfo.RawArguments) +func BenchmarkParseMCPMethodInfo(b *testing.B) { + payloads := []struct { + name string + args string + }{ + {name: "small", args: `{"owner":"github","repo":"server","path":"README.md"}`}, + { + name: "large_string", + args: `{"owner":"github","repo":"server","content":"` + strings.Repeat("x", 128*1024) + `"}`, + }, + { + name: "large_nested", + args: `{"owner":"github","repo":"server","files":[` + + strings.Repeat(`{"path":"README.md","content":"`+strings.Repeat("x", 1024)+`"},`, 127) + `{}]}`, + }, + } + for _, payload := range payloads { + b.Run(payload.name, func(b *testing.B) { + body := []byte(`{"jsonrpc":"2.0","method":"tools/call","params":{"name":"test_tool","arguments":` + payload.args + `}}`) + for _, eager := range []bool{false, true} { + name := "parse_only" + if eager { + name = "eager_map_reference" + } + b.Run(name, func(b *testing.B) { + b.ReportAllocs() + b.SetBytes(int64(len(body))) + for b.Loop() { + info, err := parseMCPMethodInfo(body) + if err != nil || info == nil { + b.Fatalf("failed to parse MCP envelope: %v", err) + } + if len(info.RawArguments) != len(payload.args) { + b.Fatal("arguments were not preserved") + } + // Compare envelope-only parsing with the former eager + // map decode. Neither branch calls DecodeArguments. + if eager { + var args map[string]any + if err := json.Unmarshal(info.RawArguments, &args); err != nil { + b.Fatal(err) + } + if args["owner"] != "github" || args["repo"] != "server" { + b.Fatal("unexpected repository arguments") + } + } + } + }) + } + }) + } } func TestWithMCPParse_BodyRestoration(t *testing.T) { diff --git a/pkg/octicons/octicons.go b/pkg/octicons/octicons.go index 09e8f1275b..d0a06ed226 100644 --- a/pkg/octicons/octicons.go +++ b/pkg/octicons/octicons.go @@ -22,11 +22,12 @@ type dataURIKey struct { theme Theme } -var dataURIs = loadDataURIs() +var dataURIs = loadDataURIs(embeddedDataURIs) -func loadDataURIs() map[dataURIKey]string { +func loadDataURIs(contents string) map[dataURIKey]string { dataURIs := make(map[dataURIKey]string) - for line := range strings.SplitSeq(strings.TrimSpace(embeddedDataURIs), "\n") { + for line := range strings.SplitSeq(strings.TrimSpace(contents), "\n") { + line = strings.TrimSuffix(line, "\r") filename, dataURI, ok := strings.Cut(line, "\t") separator := strings.LastIndexByte(filename, '-') if !ok || separator <= 0 || !strings.HasPrefix(dataURI, "data:image/png;base64,") { diff --git a/pkg/octicons/octicons_test.go b/pkg/octicons/octicons_test.go index 7d9c2e99d0..32d95ce06c 100644 --- a/pkg/octicons/octicons_test.go +++ b/pkg/octicons/octicons_test.go @@ -15,6 +15,54 @@ import ( //go:embed icons/*.png var iconPNGs embed.FS +func TestLoadDataURIsLineEndings(t *testing.T) { + const lightURI = "data:image/png;base64,bGlnaHQ=" + const darkURI = "data:image/png;base64,ZGFyaw==" + for _, tc := range []struct { + name string + newline string + omitFinalNewline bool + }{ + {name: "LF", newline: "\n"}, + {name: "CRLF", newline: "\r\n"}, + {name: "LF without final newline", newline: "\n", omitFinalNewline: true}, + {name: "CRLF without final newline", newline: "\r\n", omitFinalNewline: true}, + } { + t.Run(tc.name, func(t *testing.T) { + contents := strings.Join([]string{ + "repo-light\t" + lightURI, + "repo-dark\t" + darkURI, + "", + }, tc.newline) + if tc.omitFinalNewline { + contents = strings.TrimSuffix(contents, tc.newline) + } + + assert.Equal(t, map[dataURIKey]string{ + {name: "repo", theme: ThemeLight}: lightURI, + {name: "repo", theme: ThemeDark}: darkURI, + }, loadDataURIs(contents)) + }) + } +} + +func TestLoadDataURIsInvalidEntries(t *testing.T) { + for _, tc := range []struct { + name string + contents string + }{ + {name: "empty manifest", contents: ""}, + {name: "missing tab", contents: "repo-light data:image/png;base64,bGlnaHQ="}, + {name: "missing name", contents: "-light\tdata:image/png;base64,bGlnaHQ="}, + {name: "wrong media type", contents: "repo-light\tdata:image/svg+xml;base64,bGlnaHQ="}, + {name: "unknown theme", contents: "repo-other\tdata:image/png;base64,bGlnaHQ="}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Panics(t, func() { loadDataURIs(tc.contents) }) + }) + } +} + func TestDataURI(t *testing.T) { tests := []struct { name string @@ -86,6 +134,7 @@ func TestDataURIForEveryEmbeddedIcon(t *testing.T) { require.NoError(t, err) dataURI := DataURI(name, theme) + assert.Equal(t, "data:image/png;base64,"+base64.StdEncoding.EncodeToString(png), dataURI) require.True(t, strings.HasPrefix(dataURI, "data:image/png;base64,")) encodedPNG := strings.TrimPrefix(dataURI, "data:image/png;base64,") decodedPNG, err := base64.StdEncoding.DecodeString(encodedPNG) diff --git a/script/lint b/script/lint index 5b69cbe2ff..6d8e97a8a1 100755 --- a/script/lint +++ b/script/lint @@ -8,8 +8,13 @@ BINARY=$BINDIR/golangci-lint # sync with .github/workflows/lint.yml GOLANGCI_LINT_VERSION=v2.9.0 -if [ ! -f "$BINARY" ]; then +if [ ! -f "$BINARY" ] || [ "$("$BINARY" version --short)" != "${GOLANGCI_LINT_VERSION#v}" ]; then curl -sSfL https://golangci-lint.run/install.sh | sh -s -- -b "$BINDIR" "$GOLANGCI_LINT_VERSION" fi -$BINARY run \ No newline at end of file +if [ "$("$BINARY" version --short)" != "${GOLANGCI_LINT_VERSION#v}" ]; then + echo "Expected golangci-lint $GOLANGCI_LINT_VERSION at $BINARY" >&2 + exit 1 +fi + +"$BINARY" run \ No newline at end of file