diff --git a/.github/workflows/mcp-diff.yml b/.github/workflows/mcp-diff.yml index 9c9c34b3a9..ba9b59c6e1 100644 --- a/.github/workflows/mcp-diff.yml +++ b/.github/workflows/mcp-diff.yml @@ -20,7 +20,7 @@ jobs: fetch-depth: 0 - name: Run MCP Server Diff - uses: SamMorrowDrums/mcp-server-diff@v2 + uses: SamMorrowDrums/mcp-server-diff@v2.3.5 with: setup_go: "true" install_command: go mod download @@ -35,8 +35,10 @@ jobs: {"name": "read-only+dynamic", "args": "--read-only --dynamic-toolsets"}, {"name": "toolsets-repos", "args": "--toolsets=repos"}, {"name": "toolsets-issues", "args": "--toolsets=issues"}, + {"name": "toolsets-context", "args": "--toolsets=context"}, {"name": "toolsets-pull_requests", "args": "--toolsets=pull_requests"}, {"name": "toolsets-repos,issues", "args": "--toolsets=repos,issues"}, + {"name": "toolsets-issues,context", "args": "--toolsets=issues,context"}, {"name": "toolsets-all", "args": "--toolsets=all"}, {"name": "tools-get_me", "args": "--tools=get_me"}, {"name": "tools-get_me,list_issues", "args": "--tools=get_me,list_issues"}, diff --git a/Dockerfile b/Dockerfile index 9d68a985a2..f804c03aac 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM golang:1.25.4-alpine AS build +FROM golang:1.25.6-alpine AS build ARG VERSION="dev" # Set the working directory diff --git a/pkg/buffer/buffer.go b/pkg/buffer/buffer.go index 14bcf95821..54ed0be4d8 100644 --- a/pkg/buffer/buffer.go +++ b/pkg/buffer/buffer.go @@ -1,12 +1,17 @@ package buffer import ( - "bufio" + "bytes" "fmt" + "io" "net/http" "strings" ) +// maxLineSize is the maximum size for a single log line (10MB). +// GitHub Actions logs can contain extremely long lines (base64 content, minified JS, etc.) +const maxLineSize = 10 * 1024 * 1024 + // ProcessResponseAsRingBufferToEnd reads the body of an HTTP response line by line, // storing only the last maxJobLogLines lines using a ring buffer (sliding window). // This efficiently retains the most recent lines, overwriting older ones as needed. @@ -25,6 +30,7 @@ import ( // // The function uses a ring buffer to efficiently store only the last maxJobLogLines lines. // If the response contains more lines than maxJobLogLines, only the most recent lines are kept. +// Lines exceeding maxLineSize are truncated with a marker. func ProcessResponseAsRingBufferToEnd(httpResp *http.Response, maxJobLogLines int) (string, int, *http.Response, error) { if maxJobLogLines > 100000 { maxJobLogLines = 100000 @@ -35,20 +41,74 @@ func ProcessResponseAsRingBufferToEnd(httpResp *http.Response, maxJobLogLines in totalLines := 0 writeIndex := 0 - scanner := bufio.NewScanner(httpResp.Body) - scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + const readBufferSize = 64 * 1024 // 64KB read buffer + const maxDisplayLength = 1000 // Keep first 1000 chars of truncated lines - for scanner.Scan() { - line := scanner.Text() - totalLines++ + readBuf := make([]byte, readBufferSize) + var currentLine strings.Builder + lineTruncated := false + // storeLine saves the current line to the ring buffer and resets state + storeLine := func() { + line := currentLine.String() + if lineTruncated && len(line) > maxDisplayLength { + line = line[:maxDisplayLength] + } + if lineTruncated { + line += "... [TRUNCATED]" + } lines[writeIndex] = line validLines[writeIndex] = true + totalLines++ writeIndex = (writeIndex + 1) % maxJobLogLines + currentLine.Reset() + lineTruncated = false } - if err := scanner.Err(); err != nil { - return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) + // accumulate adds bytes to currentLine up to maxLineSize, sets lineTruncated if exceeded + accumulate := func(data []byte) { + if lineTruncated { + return + } + remaining := maxLineSize - currentLine.Len() + if remaining <= 0 { + lineTruncated = true + return + } + if remaining > len(data) { + remaining = len(data) + } + currentLine.Write(data[:remaining]) + if currentLine.Len() >= maxLineSize { + lineTruncated = true + } + } + + for { + n, err := httpResp.Body.Read(readBuf) + if n > 0 { + chunk := readBuf[:n] + for len(chunk) > 0 { + newlineIdx := bytes.IndexByte(chunk, '\n') + if newlineIdx < 0 { + accumulate(chunk) + break + } + accumulate(chunk[:newlineIdx]) + storeLine() + chunk = chunk[newlineIdx+1:] + } + } + + if err == io.EOF { + if currentLine.Len() > 0 { + storeLine() + } + break + } + if err != nil { + return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) + } } var result []string diff --git a/pkg/buffer/buffer_test.go b/pkg/buffer/buffer_test.go new file mode 100644 index 0000000000..86308ec5e3 --- /dev/null +++ b/pkg/buffer/buffer_test.go @@ -0,0 +1,176 @@ +package buffer + +import ( + "fmt" + "io" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestProcessResponseAsRingBufferToEnd(t *testing.T) { + t.Run("normal lines", func(t *testing.T) { + body := "line1\nline2\nline3\n" + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(body)), + } + + result, totalLines, respOut, err := ProcessResponseAsRingBufferToEnd(resp, 10) + if respOut != nil && respOut.Body != nil { + defer respOut.Body.Close() + } + require.NoError(t, err) + assert.Equal(t, 3, totalLines) + assert.Equal(t, "line1\nline2\nline3", result) + }) + + t.Run("ring buffer keeps last N lines", func(t *testing.T) { + body := "line1\nline2\nline3\nline4\nline5\n" + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(body)), + } + + result, totalLines, respOut, err := ProcessResponseAsRingBufferToEnd(resp, 3) + if respOut != nil && respOut.Body != nil { + defer respOut.Body.Close() + } + require.NoError(t, err) + assert.Equal(t, 5, totalLines) + assert.Equal(t, "line3\nline4\nline5", result) + }) + + t.Run("handles very long line exceeding 10MB", func(t *testing.T) { + // Create a line that exceeds maxLineSize (10MB) + longLine := strings.Repeat("x", 11*1024*1024) // 11MB + body := "line1\n" + longLine + "\nline3\n" + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(body)), + } + + result, totalLines, respOut, err := ProcessResponseAsRingBufferToEnd(resp, 100) + if respOut != nil && respOut.Body != nil { + defer respOut.Body.Close() + } + require.NoError(t, err) + // Should have processed lines with truncation marker + assert.Greater(t, totalLines, 0) + assert.Contains(t, result, "TRUNCATED") + }) + + t.Run("handles line at exactly max size", func(t *testing.T) { + // Create a line just under maxLineSize + longLine := strings.Repeat("a", 1024*1024) // 1MB - should work fine + body := "start\n" + longLine + "\nend\n" + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(body)), + } + + result, totalLines, respOut, err := ProcessResponseAsRingBufferToEnd(resp, 100) + if respOut != nil && respOut.Body != nil { + defer respOut.Body.Close() + } + require.NoError(t, err) + assert.Equal(t, 3, totalLines) + assert.Contains(t, result, "start") + assert.Contains(t, result, "end") + }) + + t.Run("ring buffer with long line in middle of many lines", func(t *testing.T) { + // Create many lines with a long line in the middle + // Ring buffer size is 5, so we should only keep the last 5 lines + var sb strings.Builder + for i := 1; i <= 10; i++ { + sb.WriteString(fmt.Sprintf("line%d\n", i)) + } + // Insert an 11MB line (exceeds maxLineSize of 10MB) + longLine := strings.Repeat("x", 11*1024*1024) + sb.WriteString(longLine) + sb.WriteString("\n") + for i := 11; i <= 20; i++ { + sb.WriteString(fmt.Sprintf("line%d\n", i)) + } + + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(sb.String())), + } + + result, totalLines, respOut, err := ProcessResponseAsRingBufferToEnd(resp, 5) + if respOut != nil && respOut.Body != nil { + defer respOut.Body.Close() + } + require.NoError(t, err) + // 10 lines before + 1 long line + 10 lines after = 21 total + assert.Equal(t, 21, totalLines) + // Should only have the last 5 lines (line16 through line20) + assert.Contains(t, result, "line16") + assert.Contains(t, result, "line17") + assert.Contains(t, result, "line18") + assert.Contains(t, result, "line19") + assert.Contains(t, result, "line20") + // Should NOT contain earlier lines + assert.NotContains(t, result, "line1\n") + assert.NotContains(t, result, "line10\n") + // The truncated line should not be in the last 5 + assert.NotContains(t, result, "TRUNCATED") + }) + + t.Run("empty response body", func(t *testing.T) { + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader("")), + } + + result, totalLines, respOut, err := ProcessResponseAsRingBufferToEnd(resp, 10) + if respOut != nil && respOut.Body != nil { + defer respOut.Body.Close() + } + require.NoError(t, err) + assert.Equal(t, 0, totalLines) + assert.Equal(t, "", result) + }) + + t.Run("line at exactly maxLineSize boundary", func(t *testing.T) { + // Create a line at exactly maxLineSize (10MB) - should be truncated + exactLine := strings.Repeat("z", 10*1024*1024) + body := "before\n" + exactLine + "\nafter\n" + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(body)), + } + + result, totalLines, respOut, err := ProcessResponseAsRingBufferToEnd(resp, 10) + if respOut != nil && respOut.Body != nil { + defer respOut.Body.Close() + } + require.NoError(t, err) + assert.Equal(t, 3, totalLines) + assert.Contains(t, result, "before") + assert.Contains(t, result, "TRUNCATED") + assert.Contains(t, result, "after") + }) + + t.Run("ring buffer keeps truncated line when in last N", func(t *testing.T) { + // Long line followed by only 2 more lines, with ring buffer size 5 + longLine := strings.Repeat("y", 11*1024*1024) + body := "line1\nline2\nline3\n" + longLine + "\nlineA\nlineB\n" + resp := &http.Response{ + Body: io.NopCloser(strings.NewReader(body)), + } + + result, totalLines, respOut, err := ProcessResponseAsRingBufferToEnd(resp, 5) + if respOut != nil && respOut.Body != nil { + defer respOut.Body.Close() + } + require.NoError(t, err) + assert.Equal(t, 6, totalLines) + // Last 5: line2, line3, truncated, lineA, lineB + assert.Contains(t, result, "line2") + assert.Contains(t, result, "line3") + assert.Contains(t, result, "TRUNCATED") + assert.Contains(t, result, "lineA") + assert.Contains(t, result, "lineB") + // line1 should be rotated out + assert.NotContains(t, result, "line1") + }) +} diff --git a/pkg/inventory/instructions.go b/pkg/inventory/instructions.go index e4524eb43d..02e90cd200 100644 --- a/pkg/inventory/instructions.go +++ b/pkg/inventory/instructions.go @@ -31,7 +31,7 @@ Tool usage guidance: instructions = append(instructions, baseInstruction) // Collect instructions from each enabled toolset - for _, toolset := range inv.AvailableToolsets() { + for _, toolset := range inv.EnabledToolsets() { if toolset.InstructionsFunc != nil { if toolsetInstructions := toolset.InstructionsFunc(inv); toolsetInstructions != "" { instructions = append(instructions, toolsetInstructions) diff --git a/pkg/inventory/instructions_test.go b/pkg/inventory/instructions_test.go index 7a347c1e25..e8e369b3db 100644 --- a/pkg/inventory/instructions_test.go +++ b/pkg/inventory/instructions_test.go @@ -7,6 +7,7 @@ import ( ) // createTestInventory creates an inventory with the specified toolsets for testing. +// All toolsets are enabled by default using WithToolsets([]string{"all"}). func createTestInventory(toolsets []ToolsetMetadata) *Inventory { // Create tools for each toolset so they show up in AvailableToolsets() var tools []ServerTool @@ -18,6 +19,7 @@ func createTestInventory(toolsets []ToolsetMetadata) *Inventory { inv, _ := NewBuilder(). SetTools(tools). + WithToolsets([]string{"all"}). Build() return inv @@ -203,3 +205,61 @@ func TestToolsetInstructionsFunc(t *testing.T) { }) } } + +// TestGenerateInstructionsOnlyEnabledToolsets verifies that generateInstructions +// only includes instructions from enabled toolsets, not all available toolsets. +// This is a regression test for https://github.com/github/github-mcp-server/issues/1897 +func TestGenerateInstructionsOnlyEnabledToolsets(t *testing.T) { + // Create tools for multiple toolsets + reposToolset := ToolsetMetadata{ + ID: "repos", + Description: "Repository tools", + InstructionsFunc: func(_ *Inventory) string { + return "REPOS_INSTRUCTIONS" + }, + } + issuesToolset := ToolsetMetadata{ + ID: "issues", + Description: "Issue tools", + InstructionsFunc: func(_ *Inventory) string { + return "ISSUES_INSTRUCTIONS" + }, + } + prsToolset := ToolsetMetadata{ + ID: "pull_requests", + Description: "PR tools", + InstructionsFunc: func(_ *Inventory) string { + return "PRS_INSTRUCTIONS" + }, + } + + tools := []ServerTool{ + {Toolset: reposToolset}, + {Toolset: issuesToolset}, + {Toolset: prsToolset}, + } + + // Build inventory with only "repos" toolset enabled + inv, err := NewBuilder(). + SetTools(tools). + WithToolsets([]string{"repos"}). + Build() + if err != nil { + t.Fatalf("Failed to build inventory: %v", err) + } + + result := generateInstructions(inv) + + // Should contain instructions from enabled toolset + if !strings.Contains(result, "REPOS_INSTRUCTIONS") { + t.Errorf("Expected instructions to contain 'REPOS_INSTRUCTIONS' for enabled toolset, but it did not. Result: %s", result) + } + + // Should NOT contain instructions from non-enabled toolsets + if strings.Contains(result, "ISSUES_INSTRUCTIONS") { + t.Errorf("Did not expect instructions to contain 'ISSUES_INSTRUCTIONS' for disabled toolset, but it did. Result: %s", result) + } + if strings.Contains(result, "PRS_INSTRUCTIONS") { + t.Errorf("Did not expect instructions to contain 'PRS_INSTRUCTIONS' for disabled toolset, but it did. Result: %s", result) + } +} diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index e4113b4520..e2cd3a9e67 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -295,6 +295,28 @@ func (r *Inventory) AvailableToolsets(exclude ...ToolsetID) []ToolsetMetadata { return result } +// EnabledToolsets returns the unique toolsets that are enabled based on current filters. +// This is similar to AvailableToolsets but respects the enabledToolsets filter. +// Returns toolsets in sorted order by toolset ID. +func (r *Inventory) EnabledToolsets() []ToolsetMetadata { + // Get all available toolsets first (already sorted by ID) + allToolsets := r.AvailableToolsets() + + // If no filter is set, all toolsets are enabled + if r.enabledToolsets == nil { + return allToolsets + } + + // Filter to only enabled toolsets + var result []ToolsetMetadata + for _, ts := range allToolsets { + if r.enabledToolsets[ts.ID] { + result = append(result, ts) + } + } + return result +} + func (r *Inventory) Instructions() string { return r.instructions }