From 8a5efb986c91b665f71dced78e18314be71ea221 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 12 Aug 2025 17:04:51 +0100 Subject: [PATCH 01/40] add sliding window for actions logs --- pkg/github/actions.go | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 19b56389c..88f1e8e28 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -1,10 +1,10 @@ package github import ( + "bufio" "context" "encoding/json" "fmt" - "io" "net/http" "strconv" "strings" @@ -754,16 +754,36 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon return "", 0, httpResp, fmt.Errorf("failed to download logs: HTTP %d", httpResp.StatusCode) } - content, err := io.ReadAll(httpResp.Body) - if err != nil { - return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) + // content, err := io.ReadAll(httpResp.Body) + // if err != nil { + // return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) + // } + + if tailLines <= 0 { + tailLines = 1000 } - // Clean up and format the log content for better readability - logContent := strings.TrimSpace(string(content)) + lines := make([]string, 0, tailLines) + scanner := bufio.NewScanner(httpResp.Body) + + buf := make([]byte, 0, 64*1024) + scanner.Buffer(buf, 1024*1024) + + for scanner.Scan() { + line := scanner.Text() + lines = append(lines, line) + + if len(lines) > tailLines { + lines = lines[len(lines)-tailLines:] + } + } + + if err := scanner.Err(); err != nil { + return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) + } - trimmedContent, lineCount := trimContent(logContent, tailLines) - return trimmedContent, lineCount, httpResp, nil + content := strings.Join(lines, "\n") + return content, len(lines), httpResp, nil } // trimContent trims the content to a maximum length and returns the trimmed content and an original length From e6ef962e18c8201e39964bd93b9f3f168b6698c9 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 12 Aug 2025 17:06:45 +0100 Subject: [PATCH 02/40] refactor: fix sliding --- pkg/github/actions.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 88f1e8e28..a9f7b59ef 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -754,11 +754,6 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon return "", 0, httpResp, fmt.Errorf("failed to download logs: HTTP %d", httpResp.StatusCode) } - // content, err := io.ReadAll(httpResp.Body) - // if err != nil { - // return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) - // } - if tailLines <= 0 { tailLines = 1000 } @@ -774,7 +769,7 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon lines = append(lines, line) if len(lines) > tailLines { - lines = lines[len(lines)-tailLines:] + lines = lines[1:] } } From 271c7c2d1e1199e256173a7451558bb2a2d04e13 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 12 Aug 2025 17:06:54 +0100 Subject: [PATCH 03/40] remove trim content --- pkg/github/actions.go | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index a9f7b59ef..51a5e416d 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -781,26 +781,6 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon return content, len(lines), httpResp, nil } -// trimContent trims the content to a maximum length and returns the trimmed content and an original length -func trimContent(content string, tailLines int) (string, int) { - // Truncate to tail_lines if specified - lineCount := 0 - if tailLines > 0 { - - // Count backwards to find the nth newline from the end and a total number of lines - for i := len(content) - 1; i >= 0 && lineCount < tailLines; i-- { - if content[i] == '\n' { - lineCount++ - // If we have reached the tailLines, trim the content - if lineCount == tailLines { - content = content[i+1:] - } - } - } - } - return content, lineCount -} - // RerunWorkflowRun creates a tool to re-run an entire workflow run func RerunWorkflowRun(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("rerun_workflow_run", From e65c0f01fd728ca9e4d22d80e7496bf5f609dd7e Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Tue, 12 Aug 2025 17:21:14 +0100 Subject: [PATCH 04/40] only use up to 1mb of memory for logs --- pkg/github/actions.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 51a5e416d..a3abc6024 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -744,7 +744,7 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin // downloadLogContent downloads the actual log content from a GitHub logs URL func downloadLogContent(logURL string, tailLines int) (string, int, *http.Response, error) { - httpResp, err := http.Get(logURL) //nolint:gosec // URLs are provided by GitHub API and are safe + httpResp, err := http.Get(logURL) //nolint:gosec if err != nil { return "", 0, httpResp, fmt.Errorf("failed to download logs: %w", err) } @@ -758,27 +758,35 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - lines := make([]string, 0, tailLines) - scanner := bufio.NewScanner(httpResp.Body) + const maxMemoryBytes = 1024 * 1024 + var lines []string + var currentMemoryUsage int + totalLines := 0 - buf := make([]byte, 0, 64*1024) - scanner.Buffer(buf, 1024*1024) + scanner := bufio.NewScanner(httpResp.Body) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) for scanner.Scan() { line := scanner.Text() - lines = append(lines, line) + totalLines++ + lineSize := len(line) + 1 - if len(lines) > tailLines { + // Remove old lines if we exceed memory limit or line count limit + for (currentMemoryUsage+lineSize > maxMemoryBytes || len(lines) >= tailLines) && len(lines) > 0 { + removedLineSize := len(lines[0]) + 1 + currentMemoryUsage -= removedLineSize lines = lines[1:] } + + lines = append(lines, line) + currentMemoryUsage += lineSize } if err := scanner.Err(); err != nil { return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) } - content := strings.Join(lines, "\n") - return content, len(lines), httpResp, nil + return strings.Join(lines, "\n"), totalLines, httpResp, nil } // RerunWorkflowRun creates a tool to re-run an entire workflow run From ade3852a2cd0ff28c8dacbd58b9b577e59085100 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 13 Aug 2025 09:35:52 +0100 Subject: [PATCH 05/40] update to tail lines in second pass --- pkg/github/actions.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index a3abc6024..55f6c325b 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -758,7 +758,7 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - const maxMemoryBytes = 1024 * 1024 + const maxMemoryBytes = 1024 * 1024 // 1MB memory limit var lines []string var currentMemoryUsage int totalLines := 0 @@ -769,13 +769,14 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon for scanner.Scan() { line := scanner.Text() totalLines++ - lineSize := len(line) + 1 + lineSize := len(line) + 1 // +1 for newline character - // Remove old lines if we exceed memory limit or line count limit - for (currentMemoryUsage+lineSize > maxMemoryBytes || len(lines) >= tailLines) && len(lines) > 0 { - removedLineSize := len(lines[0]) + 1 - currentMemoryUsage -= removedLineSize - lines = lines[1:] + if currentMemoryUsage+lineSize > maxMemoryBytes { + for currentMemoryUsage+lineSize > maxMemoryBytes && len(lines) > 0 { + removedLineSize := len(lines[0]) + 1 + currentMemoryUsage -= removedLineSize + lines = lines[1:] + } } lines = append(lines, line) @@ -786,6 +787,10 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) } + if len(lines) > tailLines { + lines = lines[len(lines)-tailLines:] + } + return strings.Join(lines, "\n"), totalLines, httpResp, nil } From 5a76cbd408374e7119d4ad9d45902e54592e512b Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 13 Aug 2025 09:52:44 +0100 Subject: [PATCH 06/40] add better memory usage calculation --- pkg/github/actions.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 55f6c325b..79cb03f51 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -758,29 +758,34 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - const maxMemoryBytes = 1024 * 1024 // 1MB memory limit + const maxMemoryBytes = 1024 * 1024 var lines []string - var currentMemoryUsage int totalLines := 0 + calculateMemoryUsage := func() int { + total := 0 + for _, line := range lines { + total += len(line) + 1 + } + return total + } + scanner := bufio.NewScanner(httpResp.Body) scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) for scanner.Scan() { line := scanner.Text() totalLines++ - lineSize := len(line) + 1 // +1 for newline character + lineSize := len(line) + 1 + currentMemoryUsage := calculateMemoryUsage() if currentMemoryUsage+lineSize > maxMemoryBytes { - for currentMemoryUsage+lineSize > maxMemoryBytes && len(lines) > 0 { - removedLineSize := len(lines[0]) + 1 - currentMemoryUsage -= removedLineSize + for calculateMemoryUsage()+lineSize > maxMemoryBytes && len(lines) > 0 { lines = lines[1:] } } lines = append(lines, line) - currentMemoryUsage += lineSize } if err := scanner.Err(); err != nil { From 8e60fb2e3a803707285ab69f3cb6d02b2144b956 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 13 Aug 2025 13:52:44 +0100 Subject: [PATCH 07/40] increase window size to 5MB --- pkg/github/actions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 79cb03f51..3a47153fd 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -758,7 +758,7 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - const maxMemoryBytes = 1024 * 1024 + const maxMemoryBytes = 5 * 1024 * 1024 var lines []string totalLines := 0 From e1c314319c591ad51268a4a74f264039013582a9 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Wed, 13 Aug 2025 13:55:04 +0100 Subject: [PATCH 08/40] update test --- pkg/github/actions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index cb33cbe6b..0ae785b8a 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -1139,7 +1139,7 @@ func Test_GetJobLogs_WithContentReturnAndTailLines(t *testing.T) { require.NoError(t, err) assert.Equal(t, float64(123), response["job_id"]) - assert.Equal(t, float64(1), response["original_length"]) + assert.Equal(t, float64(3), response["original_length"]) assert.Equal(t, expectedLogContent, response["logs_content"]) assert.Equal(t, "Job logs content retrieved successfully", response["message"]) assert.NotContains(t, response, "logs_url") // Should not have URL when returning content From 6b8f2bafe06377bebc18e9b4f43d728a99a1afd4 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 12:46:53 +0100 Subject: [PATCH 09/40] update vers --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index b566e6c40..429806d29 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.23.7 require ( github.com/google/go-github/v74 v74.0.0 github.com/josephburnett/jd v1.9.2 - github.com/mark3labs/mcp-go v0.36.0 + github.com/mark3labs/mcp-go v0.37.0 github.com/migueleliasweb/go-github-mock v1.3.0 github.com/spf13/cobra v1.9.1 github.com/spf13/viper v1.20.1 diff --git a/go.sum b/go.sum index 24377c8aa..d5bfbddf5 100644 --- a/go.sum +++ b/go.sum @@ -55,6 +55,8 @@ github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0 github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= github.com/mark3labs/mcp-go v0.36.0 h1:rIZaijrRYPeSbJG8/qNDe0hWlGrCJ7FWHNMz2SQpTis= github.com/mark3labs/mcp-go v0.36.0/go.mod h1:T7tUa2jO6MavG+3P25Oy/jR7iCeJPHImCZHRymCn39g= +github.com/mark3labs/mcp-go v0.37.0 h1:BywvZLPRT6Zx6mMG/MJfxLSZQkTGIcJSEGKsvr4DsoQ= +github.com/mark3labs/mcp-go v0.37.0/go.mod h1:T7tUa2jO6MavG+3P25Oy/jR7iCeJPHImCZHRymCn39g= github.com/migueleliasweb/go-github-mock v1.3.0 h1:2sVP9JEMB2ubQw1IKto3/fzF51oFC6eVWOOFDgQoq88= github.com/migueleliasweb/go-github-mock v1.3.0/go.mod h1:ipQhV8fTcj/G6m7BKzin08GaJ/3B5/SonRAkgrk0zCY= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= From 8002fbd44b165bdfc402252ef8bc3235cea6dec2 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 12:55:57 +0100 Subject: [PATCH 10/40] undo vers change --- go.mod | 2 +- go.sum | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 429806d29..b566e6c40 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.23.7 require ( github.com/google/go-github/v74 v74.0.0 github.com/josephburnett/jd v1.9.2 - github.com/mark3labs/mcp-go v0.37.0 + github.com/mark3labs/mcp-go v0.36.0 github.com/migueleliasweb/go-github-mock v1.3.0 github.com/spf13/cobra v1.9.1 github.com/spf13/viper v1.20.1 diff --git a/go.sum b/go.sum index d5bfbddf5..24377c8aa 100644 --- a/go.sum +++ b/go.sum @@ -55,8 +55,6 @@ github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0 github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= github.com/mark3labs/mcp-go v0.36.0 h1:rIZaijrRYPeSbJG8/qNDe0hWlGrCJ7FWHNMz2SQpTis= github.com/mark3labs/mcp-go v0.36.0/go.mod h1:T7tUa2jO6MavG+3P25Oy/jR7iCeJPHImCZHRymCn39g= -github.com/mark3labs/mcp-go v0.37.0 h1:BywvZLPRT6Zx6mMG/MJfxLSZQkTGIcJSEGKsvr4DsoQ= -github.com/mark3labs/mcp-go v0.37.0/go.mod h1:T7tUa2jO6MavG+3P25Oy/jR7iCeJPHImCZHRymCn39g= github.com/migueleliasweb/go-github-mock v1.3.0 h1:2sVP9JEMB2ubQw1IKto3/fzF51oFC6eVWOOFDgQoq88= github.com/migueleliasweb/go-github-mock v1.3.0/go.mod h1:ipQhV8fTcj/G6m7BKzin08GaJ/3B5/SonRAkgrk0zCY= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= From 52e531e4c034cbe0839f47bf12a97eeee154c5ab Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 13:36:11 +0100 Subject: [PATCH 11/40] add incremental memory tracking --- pkg/github/actions.go | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index bd44b30c8..76a128748 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -761,14 +761,7 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon const maxMemoryBytes = 5 * 1024 * 1024 var lines []string totalLines := 0 - - calculateMemoryUsage := func() int { - total := 0 - for _, line := range lines { - total += len(line) + 1 - } - return total - } + currentMemoryUsage := 0 scanner := bufio.NewScanner(httpResp.Body) scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) @@ -778,14 +771,16 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon totalLines++ lineSize := len(line) + 1 - currentMemoryUsage := calculateMemoryUsage() - if currentMemoryUsage+lineSize > maxMemoryBytes { - for calculateMemoryUsage()+lineSize > maxMemoryBytes && len(lines) > 0 { - lines = lines[1:] - } + // Remove lines from the front until we have space for the new line + for currentMemoryUsage+lineSize > maxMemoryBytes && len(lines) > 0 { + removedLineSize := len(lines[0]) + 1 + currentMemoryUsage -= removedLineSize + lines = lines[1:] } + // Add the new line lines = append(lines, line) + currentMemoryUsage += lineSize } if err := scanner.Err(); err != nil { From 8f8539831ccd0081247adaa97d586e11e90f0393 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 15:12:27 +0100 Subject: [PATCH 12/40] use ring buffer --- pkg/github/actions.go | 54 ++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 19 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 76a128748..e076aa2b2 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "net/http" + "runtime" "strconv" "strings" @@ -721,7 +722,7 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin if returnContent { // Download and return the actual log content - content, originalLength, httpResp, err := downloadLogContent(url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp + content, originalLength, httpResp, err := downloadLogContent(ctx, url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp if err != nil { // To keep the return value consistent wrap the response as a GitHub Response ghRes := &github.Response{ @@ -742,8 +743,7 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin return result, resp, nil } -// downloadLogContent downloads the actual log content from a GitHub logs URL -func downloadLogContent(logURL string, tailLines int) (string, int, *http.Response, error) { +func downloadLogContent(ctx context.Context, logURL string, tailLines int) (string, int, *http.Response, error) { httpResp, err := http.Get(logURL) //nolint:gosec if err != nil { return "", 0, httpResp, fmt.Errorf("failed to download logs: %w", err) @@ -758,10 +758,11 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - const maxMemoryBytes = 5 * 1024 * 1024 - var lines []string + const maxLines = 50000 + + lines := make([]string, maxLines) totalLines := 0 - currentMemoryUsage := 0 + writeIndex := 0 scanner := bufio.NewScanner(httpResp.Body) scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) @@ -769,29 +770,44 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon for scanner.Scan() { line := scanner.Text() totalLines++ - lineSize := len(line) + 1 - // Remove lines from the front until we have space for the new line - for currentMemoryUsage+lineSize > maxMemoryBytes && len(lines) > 0 { - removedLineSize := len(lines[0]) + 1 - currentMemoryUsage -= removedLineSize - lines = lines[1:] - } + lines[writeIndex] = line + writeIndex = (writeIndex + 1) % maxLines - // Add the new line - lines = append(lines, line) - currentMemoryUsage += lineSize + if totalLines%10000 == 0 { + runtime.GC() + } } if err := scanner.Err(); err != nil { return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) } - if len(lines) > tailLines { - lines = lines[len(lines)-tailLines:] + var result []string + linesInBuffer := totalLines + if linesInBuffer > maxLines { + linesInBuffer = maxLines } - return strings.Join(lines, "\n"), totalLines, httpResp, nil + startIndex := 0 + if totalLines > maxLines { + startIndex = writeIndex + } + + for i := 0; i < linesInBuffer; i++ { + idx := (startIndex + i) % maxLines + if lines[idx] != "" { + result = append(result, lines[idx]) + } + } + + if len(result) > tailLines { + result = result[len(result)-tailLines:] + } + + finalResult := strings.Join(result, "\n") + + return finalResult, totalLines, httpResp, nil } // RerunWorkflowRun creates a tool to re-run an entire workflow run From 0d19480df6760c560e9f700ed8f759206a0acbcd Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 15:15:49 +0100 Subject: [PATCH 13/40] remove unused ctx param --- pkg/github/actions.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index e076aa2b2..00ad68cb6 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -722,7 +722,7 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin if returnContent { // Download and return the actual log content - content, originalLength, httpResp, err := downloadLogContent(ctx, url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp + content, originalLength, httpResp, err := downloadLogContent(url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp if err != nil { // To keep the return value consistent wrap the response as a GitHub Response ghRes := &github.Response{ @@ -743,7 +743,7 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin return result, resp, nil } -func downloadLogContent(ctx context.Context, logURL string, tailLines int) (string, int, *http.Response, error) { +func downloadLogContent(logURL string, tailLines int) (string, int, *http.Response, error) { httpResp, err := http.Get(logURL) //nolint:gosec if err != nil { return "", 0, httpResp, fmt.Errorf("failed to download logs: %w", err) From f104e672e8ae132dbb0e160e00e40bc436e26eca Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 15:18:53 +0100 Subject: [PATCH 14/40] remove manual GC clear --- pkg/github/actions.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 00ad68cb6..397b8a075 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -6,7 +6,6 @@ import ( "encoding/json" "fmt" "net/http" - "runtime" "strconv" "strings" @@ -773,10 +772,6 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon lines[writeIndex] = line writeIndex = (writeIndex + 1) % maxLines - - if totalLines%10000 == 0 { - runtime.GC() - } } if err := scanner.Err(); err != nil { From 9d273b9d21cbc4b2d7558307a5ef4418d640be45 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 15:22:56 +0100 Subject: [PATCH 15/40] fix cca feedback --- pkg/github/actions.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 397b8a075..6a56a13a8 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -19,6 +19,7 @@ import ( const ( DescriptionRepositoryOwner = "Repository owner" DescriptionRepositoryName = "Repository name" + maxJobLogLines = 50000 ) // ListWorkflows creates a tool to list workflows in a repository @@ -757,9 +758,8 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - const maxLines = 50000 - - lines := make([]string, maxLines) + lines := make([]string, maxJobLogLines) + validLines := make([]bool, maxJobLogLines) totalLines := 0 writeIndex := 0 @@ -771,7 +771,8 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon totalLines++ lines[writeIndex] = line - writeIndex = (writeIndex + 1) % maxLines + validLines[writeIndex] = true + writeIndex = (writeIndex + 1) % maxJobLogLines } if err := scanner.Err(); err != nil { @@ -780,18 +781,18 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon var result []string linesInBuffer := totalLines - if linesInBuffer > maxLines { - linesInBuffer = maxLines + if linesInBuffer > maxJobLogLines { + linesInBuffer = maxJobLogLines } startIndex := 0 - if totalLines > maxLines { + if totalLines > maxJobLogLines { startIndex = writeIndex } for i := 0; i < linesInBuffer; i++ { - idx := (startIndex + i) % maxLines - if lines[idx] != "" { + idx := (startIndex + i) % maxJobLogLines + if validLines[idx] { result = append(result, lines[idx]) } } From 4e43327dae35b0205cfc5f903d91d1712182b1f2 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:01:02 +0100 Subject: [PATCH 16/40] extract ring buffer logic to new package --- pkg/buffer/buffer.go | 51 +++++++++++++++++++++++++++++++++++++++++++ pkg/github/actions.go | 48 +++++----------------------------------- 2 files changed, 56 insertions(+), 43 deletions(-) create mode 100644 pkg/buffer/buffer.go diff --git a/pkg/buffer/buffer.go b/pkg/buffer/buffer.go new file mode 100644 index 000000000..e7ab3d9c1 --- /dev/null +++ b/pkg/buffer/buffer.go @@ -0,0 +1,51 @@ +package buffer + +import ( + "bufio" + "fmt" + "net/http" + "strings" +) + +func ProcessAsRingBufferToEnd(httpResp *http.Response, maxJobLogLines int) (string, int, *http.Response, error) { + lines := make([]string, maxJobLogLines) + validLines := make([]bool, maxJobLogLines) + totalLines := 0 + writeIndex := 0 + + scanner := bufio.NewScanner(httpResp.Body) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + + for scanner.Scan() { + line := scanner.Text() + totalLines++ + + lines[writeIndex] = line + validLines[writeIndex] = true + writeIndex = (writeIndex + 1) % maxJobLogLines + } + + if err := scanner.Err(); err != nil { + return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) + } + + var result []string + linesInBuffer := totalLines + if linesInBuffer > maxJobLogLines { + linesInBuffer = maxJobLogLines + } + + startIndex := 0 + if totalLines > maxJobLogLines { + startIndex = writeIndex + } + + for i := 0; i < linesInBuffer; i++ { + idx := (startIndex + i) % maxJobLogLines + if validLines[idx] { + result = append(result, lines[idx]) + } + } + + return strings.Join(result, "\n"), totalLines, httpResp, nil +} diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 6a56a13a8..6b01584f8 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -1,14 +1,13 @@ package github import ( - "bufio" "context" "encoding/json" "fmt" "net/http" "strconv" - "strings" + buffer "github.com/github/github-mcp-server/pkg/buffer" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v74/github" @@ -758,50 +757,13 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - lines := make([]string, maxJobLogLines) - validLines := make([]bool, maxJobLogLines) - totalLines := 0 - writeIndex := 0 + processedInput, totalLines, httpResp, err := buffer.ProcessAsRingBufferToEnd(httpResp, tailLines) - scanner := bufio.NewScanner(httpResp.Body) - scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) - - for scanner.Scan() { - line := scanner.Text() - totalLines++ - - lines[writeIndex] = line - validLines[writeIndex] = true - writeIndex = (writeIndex + 1) % maxJobLogLines - } - - if err := scanner.Err(); err != nil { - return "", 0, httpResp, fmt.Errorf("failed to read log content: %w", err) - } - - var result []string - linesInBuffer := totalLines - if linesInBuffer > maxJobLogLines { - linesInBuffer = maxJobLogLines - } - - startIndex := 0 - if totalLines > maxJobLogLines { - startIndex = writeIndex - } - - for i := 0; i < linesInBuffer; i++ { - idx := (startIndex + i) % maxJobLogLines - if validLines[idx] { - result = append(result, lines[idx]) - } - } - - if len(result) > tailLines { - result = result[len(result)-tailLines:] + if len(processedInput) > tailLines { + processedInput = processedInput[len(processedInput)-tailLines:] } - finalResult := strings.Join(result, "\n") + finalResult := processedInput return finalResult, totalLines, httpResp, nil } From 2ff2d4f010e844450e8f0ddc0f25171a0287d861 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:04:19 +0100 Subject: [PATCH 17/40] handle log content processing errors and use correct param for maxjobloglines --- pkg/github/actions.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 6b01584f8..acd5be004 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -757,7 +757,10 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - processedInput, totalLines, httpResp, err := buffer.ProcessAsRingBufferToEnd(httpResp, tailLines) + processedInput, totalLines, httpResp, err := buffer.ProcessAsRingBufferToEnd(httpResp, maxJobLogLines) + if err != nil { + return "", 0, httpResp, fmt.Errorf("failed to process log content: %w", err) + } if len(processedInput) > tailLines { processedInput = processedInput[len(processedInput)-tailLines:] From c6f5f7fddad9d1f94f0dd2c42cd3c2ee468330dc Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:10:10 +0100 Subject: [PATCH 18/40] fix tailing --- pkg/github/actions.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index acd5be004..b552260df 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "strconv" + "strings" buffer "github.com/github/github-mcp-server/pkg/buffer" ghErrors "github.com/github/github-mcp-server/pkg/errors" @@ -762,11 +763,11 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon return "", 0, httpResp, fmt.Errorf("failed to process log content: %w", err) } - if len(processedInput) > tailLines { - processedInput = processedInput[len(processedInput)-tailLines:] + lines := strings.Split(processedInput, "\n") + if len(lines) > tailLines { + lines = lines[len(lines)-tailLines:] } - - finalResult := processedInput + finalResult := strings.Join(lines, "\n") return finalResult, totalLines, httpResp, nil } From 1c1061c3ed814eae33492393ed7d5e0af612ed6b Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:14:12 +0100 Subject: [PATCH 19/40] account for if tailLines exceeds window size --- pkg/github/actions.go | 7 +++++- pkg/github/actions_test.go | 47 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index b552260df..d3cca63d2 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -758,7 +758,12 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon tailLines = 1000 } - processedInput, totalLines, httpResp, err := buffer.ProcessAsRingBufferToEnd(httpResp, maxJobLogLines) + bufferSize := tailLines + if bufferSize > maxJobLogLines { + bufferSize = maxJobLogLines + } + + processedInput, totalLines, httpResp, err := buffer.ProcessAsRingBufferToEnd(httpResp, bufferSize) if err != nil { return "", 0, httpResp, fmt.Errorf("failed to process log content: %w", err) } diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index ecd3387a2..8c4ae8eb1 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -1167,3 +1167,50 @@ func Test_GetJobLogs_WithContentReturnAndTailLines(t *testing.T) { assert.Equal(t, "Job logs content retrieved successfully", response["message"]) assert.NotContains(t, response, "logs_url") // Should not have URL when returning content } + +func Test_GetJobLogs_WithContentReturnAndLargeTailLines(t *testing.T) { + logContent := "Line 1\nLine 2\nLine 3" + expectedLogContent := "Line 1\nLine 2\nLine 3" + + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(logContent)) + })) + defer testServer.Close() + + mockedClient := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetReposActionsJobsLogsByOwnerByRepoByJobId, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", testServer.URL) + w.WriteHeader(http.StatusFound) + }), + ), + ) + + client := github.NewClient(mockedClient) + _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "job_id": float64(123), + "return_content": true, + "tail_lines": float64(100), + }) + + result, err := handler(context.Background(), request) + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + var response map[string]any + err = json.Unmarshal([]byte(textContent.Text), &response) + require.NoError(t, err) + + assert.Equal(t, float64(123), response["job_id"]) + assert.Equal(t, float64(3), response["original_length"]) + assert.Equal(t, expectedLogContent, response["logs_content"]) + assert.Equal(t, "Job logs content retrieved successfully", response["message"]) + assert.NotContains(t, response, "logs_url") +} From 75b8c94bf6ba6c1e839f33258e3f0ee1201a5355 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:34:44 +0100 Subject: [PATCH 20/40] add profiling thats reusable --- pkg/buffer/buffer.go | 2 +- pkg/github/actions.go | 12 ++- pkg/github/actions_test.go | 70 ++++++++++++ pkg/profiler/profiler.go | 195 ++++++++++++++++++++++++++++++++++ pkg/profiler/profiler_test.go | 183 +++++++++++++++++++++++++++++++ 5 files changed, 458 insertions(+), 4 deletions(-) create mode 100644 pkg/profiler/profiler.go create mode 100644 pkg/profiler/profiler_test.go diff --git a/pkg/buffer/buffer.go b/pkg/buffer/buffer.go index e7ab3d9c1..de7882e41 100644 --- a/pkg/buffer/buffer.go +++ b/pkg/buffer/buffer.go @@ -7,7 +7,7 @@ import ( "strings" ) -func ProcessAsRingBufferToEnd(httpResp *http.Response, maxJobLogLines int) (string, int, *http.Response, error) { +func ProcessResponseAsRingBufferToEnd(httpResp *http.Response, maxJobLogLines int) (string, int, *http.Response, error) { lines := make([]string, maxJobLogLines) validLines := make([]bool, maxJobLogLines) totalLines := 0 diff --git a/pkg/github/actions.go b/pkg/github/actions.go index d3cca63d2..855a01b53 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -10,6 +10,7 @@ import ( buffer "github.com/github/github-mcp-server/pkg/buffer" ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/profiler" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v74/github" "github.com/mark3labs/mcp-go/mcp" @@ -722,7 +723,7 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin if returnContent { // Download and return the actual log content - content, originalLength, httpResp, err := downloadLogContent(url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp + content, originalLength, httpResp, err := downloadLogContent(ctx, url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp if err != nil { // To keep the return value consistent wrap the response as a GitHub Response ghRes := &github.Response{ @@ -743,7 +744,10 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin return result, resp, nil } -func downloadLogContent(logURL string, tailLines int) (string, int, *http.Response, error) { +func downloadLogContent(ctx context.Context, logURL string, tailLines int) (string, int, *http.Response, error) { + prof := profiler.New(nil, profiler.IsProfilingEnabled()) + finish := prof.Start(ctx, "log_buffer_processing") + httpResp, err := http.Get(logURL) //nolint:gosec if err != nil { return "", 0, httpResp, fmt.Errorf("failed to download logs: %w", err) @@ -763,7 +767,7 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon bufferSize = maxJobLogLines } - processedInput, totalLines, httpResp, err := buffer.ProcessAsRingBufferToEnd(httpResp, bufferSize) + processedInput, totalLines, httpResp, err := buffer.ProcessResponseAsRingBufferToEnd(httpResp, bufferSize) if err != nil { return "", 0, httpResp, fmt.Errorf("failed to process log content: %w", err) } @@ -774,6 +778,8 @@ func downloadLogContent(logURL string, tailLines int) (string, int, *http.Respon } finalResult := strings.Join(lines, "\n") + _ = finish(len(lines), int64(len(finalResult))) + return finalResult, totalLines, httpResp, nil } diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index 8c4ae8eb1..1186185eb 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -3,10 +3,17 @@ package github import ( "context" "encoding/json" + "io" "net/http" "net/http/httptest" + "os" + "runtime" + "runtime/debug" + "strings" "testing" + buffer "github.com/github/github-mcp-server/pkg/buffer" + "github.com/github/github-mcp-server/pkg/profiler" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v74/github" "github.com/migueleliasweb/go-github-mock/src/mock" @@ -1214,3 +1221,66 @@ func Test_GetJobLogs_WithContentReturnAndLargeTailLines(t *testing.T) { assert.Equal(t, "Job logs content retrieved successfully", response["message"]) assert.NotContains(t, response, "logs_url") } + +func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { + if testing.Short() { + t.Skip("Skipping memory profiling test in short mode") + } + + const logLines = 100000 + const bufferSize = 1000 + largeLogContent := strings.Repeat("log line with some content\n", logLines) + + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(largeLogContent)) + })) + defer testServer.Close() + + os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "true") + defer os.Unsetenv("GITHUB_MCP_PROFILING_ENABLED") + + // Initialize the global profiler + profiler.InitFromEnv(nil) + + ctx := context.Background() + + debug.SetGCPercent(-1) + profile1, err1 := profiler.ProfileFuncWithMetrics(ctx, "sliding_window", func() (int, int64, error) { + resp1, err := http.Get(testServer.URL) + if err != nil { + return 0, 0, err + } + defer resp1.Body.Close() + content, totalLines, _, err := buffer.ProcessResponseAsRingBufferToEnd(resp1, bufferSize) + return totalLines, int64(len(content)), err + }) + require.NoError(t, err1) + + runtime.GC() + profile2, err2 := profiler.ProfileFuncWithMetrics(ctx, "no_window", func() (int, int64, error) { + resp2, err := http.Get(testServer.URL) + if err != nil { + return 0, 0, err + } + defer resp2.Body.Close() + content, err := io.ReadAll(resp2.Body) + if err != nil { + return 0, 0, err + } + lines := strings.Split(string(content), "\n") + if len(lines) > bufferSize { + lines = lines[len(lines)-bufferSize:] + } + result := strings.Join(lines, "\n") + return len(strings.Split(string(content), "\n")), int64(len(result)), nil + }) + require.NoError(t, err2) + debug.SetGCPercent(100) + + assert.Greater(t, profile2.MemoryDelta, profile1.MemoryDelta, + "Sliding window should use less memory than reading all into memory") + + t.Logf("Sliding window: %s", profile1.String()) + t.Logf("No window: %s", profile2.String()) +} diff --git a/pkg/profiler/profiler.go b/pkg/profiler/profiler.go new file mode 100644 index 000000000..1ebc0c2e6 --- /dev/null +++ b/pkg/profiler/profiler.go @@ -0,0 +1,195 @@ +package profiler + +import ( + "context" + "fmt" + "os" + "runtime" + "strconv" + "time" + + "log/slog" +) + +// Profile represents performance metrics for an operation +type Profile struct { + Operation string `json:"operation"` + Duration time.Duration `json:"duration_ns"` + MemoryBefore uint64 `json:"memory_before_bytes"` + MemoryAfter uint64 `json:"memory_after_bytes"` + MemoryDelta int64 `json:"memory_delta_bytes"` + LinesCount int `json:"lines_count,omitempty"` + BytesCount int64 `json:"bytes_count,omitempty"` + Timestamp time.Time `json:"timestamp"` +} + +// String returns a human-readable representation of the profile +func (p *Profile) String() string { + return fmt.Sprintf("[%s] %s: duration=%v, memory_delta=%+dB, lines=%d, bytes=%d", + p.Timestamp.Format("15:04:05.000"), + p.Operation, + p.Duration, + p.MemoryDelta, + p.LinesCount, + p.BytesCount, + ) +} + +// Profiler provides minimal performance profiling capabilities +type Profiler struct { + logger *slog.Logger + enabled bool +} + +// New creates a new Profiler instance +func New(logger *slog.Logger, enabled bool) *Profiler { + return &Profiler{ + logger: logger, + enabled: enabled, + } +} + +// ProfileFunc profiles a function execution +func (p *Profiler) ProfileFunc(ctx context.Context, operation string, fn func() error) (*Profile, error) { + if !p.enabled { + return nil, fn() + } + + profile := &Profile{ + Operation: operation, + Timestamp: time.Now(), + } + + var memBefore runtime.MemStats + runtime.ReadMemStats(&memBefore) + profile.MemoryBefore = memBefore.Alloc + + start := time.Now() + err := fn() + profile.Duration = time.Since(start) + + var memAfter runtime.MemStats + runtime.ReadMemStats(&memAfter) + profile.MemoryAfter = memAfter.Alloc + profile.MemoryDelta = int64(memAfter.Alloc) - int64(memBefore.Alloc) + + if p.logger != nil { + p.logger.InfoContext(ctx, "Performance profile", "profile", profile.String()) + } + + return profile, err +} + +// ProfileFuncWithMetrics profiles a function execution and captures additional metrics +func (p *Profiler) ProfileFuncWithMetrics(ctx context.Context, operation string, fn func() (int, int64, error)) (*Profile, error) { + if !p.enabled { + _, _, err := fn() + return nil, err + } + + profile := &Profile{ + Operation: operation, + Timestamp: time.Now(), + } + + var memBefore runtime.MemStats + runtime.ReadMemStats(&memBefore) + profile.MemoryBefore = memBefore.Alloc + + start := time.Now() + lines, bytes, err := fn() + profile.Duration = time.Since(start) + profile.LinesCount = lines + profile.BytesCount = bytes + + var memAfter runtime.MemStats + runtime.ReadMemStats(&memAfter) + profile.MemoryAfter = memAfter.Alloc + profile.MemoryDelta = int64(memAfter.Alloc) - int64(memBefore.Alloc) + + if p.logger != nil { + p.logger.InfoContext(ctx, "Performance profile", "profile", profile.String()) + } + + return profile, err +} + +// Start begins timing an operation and returns a function to complete the profiling +func (p *Profiler) Start(ctx context.Context, operation string) func(lines int, bytes int64) *Profile { + if !p.enabled { + return func(int, int64) *Profile { return nil } + } + + profile := &Profile{ + Operation: operation, + Timestamp: time.Now(), + } + + var memBefore runtime.MemStats + runtime.ReadMemStats(&memBefore) + profile.MemoryBefore = memBefore.Alloc + + start := time.Now() + + return func(lines int, bytes int64) *Profile { + profile.Duration = time.Since(start) + profile.LinesCount = lines + profile.BytesCount = bytes + + var memAfter runtime.MemStats + runtime.ReadMemStats(&memAfter) + profile.MemoryAfter = memAfter.Alloc + profile.MemoryDelta = int64(memAfter.Alloc) - int64(memBefore.Alloc) + + if p.logger != nil { + p.logger.InfoContext(ctx, "Performance profile", "profile", profile.String()) + } + + return profile + } +} + +var globalProfiler *Profiler + +// IsProfilingEnabled checks if profiling is enabled via environment variables +func IsProfilingEnabled() bool { + if enabled, err := strconv.ParseBool(os.Getenv("GITHUB_MCP_PROFILING_ENABLED")); err == nil { + return enabled + } + return false +} + +// Init initializes the global profiler +func Init(logger *slog.Logger, enabled bool) { + globalProfiler = New(logger, enabled) +} + +// InitFromEnv initializes the global profiler using environment variables +func InitFromEnv(logger *slog.Logger) { + globalProfiler = New(logger, IsProfilingEnabled()) +} + +// ProfileFunc profiles a function using the global profiler +func ProfileFunc(ctx context.Context, operation string, fn func() error) (*Profile, error) { + if globalProfiler == nil { + return nil, fn() + } + return globalProfiler.ProfileFunc(ctx, operation, fn) +} + +// ProfileFuncWithMetrics profiles a function with metrics using the global profiler +func ProfileFuncWithMetrics(ctx context.Context, operation string, fn func() (int, int64, error)) (*Profile, error) { + if globalProfiler == nil { + _, _, err := fn() + return nil, err + } + return globalProfiler.ProfileFuncWithMetrics(ctx, operation, fn) +} + +// Start begins timing using the global profiler +func Start(ctx context.Context, operation string) func(int, int64) *Profile { + if globalProfiler == nil { + return func(int, int64) *Profile { return nil } + } + return globalProfiler.Start(ctx, operation) +} diff --git a/pkg/profiler/profiler_test.go b/pkg/profiler/profiler_test.go new file mode 100644 index 000000000..ba6b90feb --- /dev/null +++ b/pkg/profiler/profiler_test.go @@ -0,0 +1,183 @@ +package profiler + +import ( + "context" + "log/slog" + "os" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestProfiler_ProfileFunc(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + profiler := New(logger, true) + + ctx := context.Background() + executed := false + + profile, err := profiler.ProfileFunc(ctx, "test_operation", func() error { + executed = true + time.Sleep(10 * time.Millisecond) + return nil + }) + + require.NoError(t, err) + assert.True(t, executed) + assert.NotNil(t, profile) + assert.Equal(t, "test_operation", profile.Operation) + assert.True(t, profile.Duration >= 10*time.Millisecond) + assert.False(t, profile.Timestamp.IsZero()) +} + +func TestProfiler_ProfileFuncWithMetrics(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + profiler := New(logger, true) + + ctx := context.Background() + executed := false + + profile, err := profiler.ProfileFuncWithMetrics(ctx, "test_operation_with_metrics", func() (int, int64, error) { + executed = true + time.Sleep(5 * time.Millisecond) + return 100, 1024, nil + }) + + require.NoError(t, err) + assert.True(t, executed) + assert.NotNil(t, profile) + assert.Equal(t, "test_operation_with_metrics", profile.Operation) + assert.True(t, profile.Duration >= 5*time.Millisecond) + assert.Equal(t, 100, profile.LinesCount) + assert.Equal(t, int64(1024), profile.BytesCount) + assert.False(t, profile.Timestamp.IsZero()) +} + +func TestProfiler_Start(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + profiler := New(logger, true) + + ctx := context.Background() + finish := profiler.Start(ctx, "test_start_operation") + + time.Sleep(5 * time.Millisecond) + profile := finish(50, 512) + + assert.NotNil(t, profile) + assert.Equal(t, "test_start_operation", profile.Operation) + assert.True(t, profile.Duration >= 5*time.Millisecond) + assert.Equal(t, 50, profile.LinesCount) + assert.Equal(t, int64(512), profile.BytesCount) + assert.False(t, profile.Timestamp.IsZero()) +} + +func TestProfiler_Disabled(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + profiler := New(logger, false) + + ctx := context.Background() + executed := false + + profile, err := profiler.ProfileFunc(ctx, "test_operation", func() error { + executed = true + return nil + }) + + require.NoError(t, err) + assert.True(t, executed) + assert.Nil(t, profile) // Should be nil when disabled +} + +func TestProfile_String(t *testing.T) { + profile := &Profile{ + Operation: "test_operation", + Duration: 100 * time.Millisecond, + MemoryDelta: 1024, + LinesCount: 100, + BytesCount: 2048, + Timestamp: time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC), + } + + str := profile.String() + assert.Contains(t, str, "test_operation") + assert.Contains(t, str, "100ms") + assert.Contains(t, str, "1024B") + assert.Contains(t, str, "lines=100") + assert.Contains(t, str, "bytes=2048") + assert.Contains(t, str, "12:00:00.000") +} + +func TestGlobalProfiler(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + Init(logger, true) + + ctx := context.Background() + executed := false + + profile, err := ProfileFunc(ctx, "global_test", func() error { + executed = true + time.Sleep(5 * time.Millisecond) + return nil + }) + + require.NoError(t, err) + assert.True(t, executed) + assert.NotNil(t, profile) + assert.Equal(t, "global_test", profile.Operation) + assert.True(t, profile.Duration >= 5*time.Millisecond) +} + +func TestMemoryProfiling(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + profiler := New(logger, true) + + ctx := context.Background() + + profile, err := profiler.ProfileFunc(ctx, "memory_allocation_test", func() error { + data := make([]string, 1000) + for i := range data { + data[i] = strings.Repeat("test", 100) + } + _ = len(data) + return nil + }) + + require.NoError(t, err) + assert.NotNil(t, profile) + assert.Equal(t, "memory_allocation_test", profile.Operation) + assert.NotEqual(t, int64(0), profile.MemoryBefore) + assert.NotEqual(t, int64(0), profile.MemoryAfter) +} + +func TestIsProfilingEnabled(t *testing.T) { + assert.False(t, IsProfilingEnabled()) + + os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "true") + defer os.Unsetenv("GITHUB_MCP_PROFILING_ENABLED") + assert.True(t, IsProfilingEnabled()) + + os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "false") + assert.False(t, IsProfilingEnabled()) + + os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "invalid") + assert.False(t, IsProfilingEnabled()) +} + +func TestInitFromEnv(t *testing.T) { + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) + + os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "true") + defer os.Unsetenv("GITHUB_MCP_PROFILING_ENABLED") + + InitFromEnv(logger) + assert.NotNil(t, globalProfiler) + assert.True(t, globalProfiler.enabled) + + os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "false") + InitFromEnv(logger) + assert.NotNil(t, globalProfiler) + assert.False(t, globalProfiler.enabled) +} From 71bfac818c060e12cbe8973e408f3303c39ac874 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:35:33 +0100 Subject: [PATCH 21/40] remove profiler testing --- pkg/profiler/profiler_test.go | 183 ---------------------------------- 1 file changed, 183 deletions(-) delete mode 100644 pkg/profiler/profiler_test.go diff --git a/pkg/profiler/profiler_test.go b/pkg/profiler/profiler_test.go deleted file mode 100644 index ba6b90feb..000000000 --- a/pkg/profiler/profiler_test.go +++ /dev/null @@ -1,183 +0,0 @@ -package profiler - -import ( - "context" - "log/slog" - "os" - "strings" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestProfiler_ProfileFunc(t *testing.T) { - logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) - profiler := New(logger, true) - - ctx := context.Background() - executed := false - - profile, err := profiler.ProfileFunc(ctx, "test_operation", func() error { - executed = true - time.Sleep(10 * time.Millisecond) - return nil - }) - - require.NoError(t, err) - assert.True(t, executed) - assert.NotNil(t, profile) - assert.Equal(t, "test_operation", profile.Operation) - assert.True(t, profile.Duration >= 10*time.Millisecond) - assert.False(t, profile.Timestamp.IsZero()) -} - -func TestProfiler_ProfileFuncWithMetrics(t *testing.T) { - logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) - profiler := New(logger, true) - - ctx := context.Background() - executed := false - - profile, err := profiler.ProfileFuncWithMetrics(ctx, "test_operation_with_metrics", func() (int, int64, error) { - executed = true - time.Sleep(5 * time.Millisecond) - return 100, 1024, nil - }) - - require.NoError(t, err) - assert.True(t, executed) - assert.NotNil(t, profile) - assert.Equal(t, "test_operation_with_metrics", profile.Operation) - assert.True(t, profile.Duration >= 5*time.Millisecond) - assert.Equal(t, 100, profile.LinesCount) - assert.Equal(t, int64(1024), profile.BytesCount) - assert.False(t, profile.Timestamp.IsZero()) -} - -func TestProfiler_Start(t *testing.T) { - logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) - profiler := New(logger, true) - - ctx := context.Background() - finish := profiler.Start(ctx, "test_start_operation") - - time.Sleep(5 * time.Millisecond) - profile := finish(50, 512) - - assert.NotNil(t, profile) - assert.Equal(t, "test_start_operation", profile.Operation) - assert.True(t, profile.Duration >= 5*time.Millisecond) - assert.Equal(t, 50, profile.LinesCount) - assert.Equal(t, int64(512), profile.BytesCount) - assert.False(t, profile.Timestamp.IsZero()) -} - -func TestProfiler_Disabled(t *testing.T) { - logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) - profiler := New(logger, false) - - ctx := context.Background() - executed := false - - profile, err := profiler.ProfileFunc(ctx, "test_operation", func() error { - executed = true - return nil - }) - - require.NoError(t, err) - assert.True(t, executed) - assert.Nil(t, profile) // Should be nil when disabled -} - -func TestProfile_String(t *testing.T) { - profile := &Profile{ - Operation: "test_operation", - Duration: 100 * time.Millisecond, - MemoryDelta: 1024, - LinesCount: 100, - BytesCount: 2048, - Timestamp: time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC), - } - - str := profile.String() - assert.Contains(t, str, "test_operation") - assert.Contains(t, str, "100ms") - assert.Contains(t, str, "1024B") - assert.Contains(t, str, "lines=100") - assert.Contains(t, str, "bytes=2048") - assert.Contains(t, str, "12:00:00.000") -} - -func TestGlobalProfiler(t *testing.T) { - logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) - Init(logger, true) - - ctx := context.Background() - executed := false - - profile, err := ProfileFunc(ctx, "global_test", func() error { - executed = true - time.Sleep(5 * time.Millisecond) - return nil - }) - - require.NoError(t, err) - assert.True(t, executed) - assert.NotNil(t, profile) - assert.Equal(t, "global_test", profile.Operation) - assert.True(t, profile.Duration >= 5*time.Millisecond) -} - -func TestMemoryProfiling(t *testing.T) { - logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) - profiler := New(logger, true) - - ctx := context.Background() - - profile, err := profiler.ProfileFunc(ctx, "memory_allocation_test", func() error { - data := make([]string, 1000) - for i := range data { - data[i] = strings.Repeat("test", 100) - } - _ = len(data) - return nil - }) - - require.NoError(t, err) - assert.NotNil(t, profile) - assert.Equal(t, "memory_allocation_test", profile.Operation) - assert.NotEqual(t, int64(0), profile.MemoryBefore) - assert.NotEqual(t, int64(0), profile.MemoryAfter) -} - -func TestIsProfilingEnabled(t *testing.T) { - assert.False(t, IsProfilingEnabled()) - - os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "true") - defer os.Unsetenv("GITHUB_MCP_PROFILING_ENABLED") - assert.True(t, IsProfilingEnabled()) - - os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "false") - assert.False(t, IsProfilingEnabled()) - - os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "invalid") - assert.False(t, IsProfilingEnabled()) -} - -func TestInitFromEnv(t *testing.T) { - logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug})) - - os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "true") - defer os.Unsetenv("GITHUB_MCP_PROFILING_ENABLED") - - InitFromEnv(logger) - assert.NotNil(t, globalProfiler) - assert.True(t, globalProfiler.enabled) - - os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "false") - InitFromEnv(logger) - assert.NotNil(t, globalProfiler) - assert.False(t, globalProfiler.enabled) -} From a43b03c67c4023bccbec295fce281b841c5fcd07 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:41:05 +0100 Subject: [PATCH 22/40] refactor profiler: introduce safeMemoryDelta for accurate memory delta calculations --- pkg/github/actions_test.go | 4 ++-- pkg/profiler/profiler.go | 27 ++++++++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index 1186185eb..bd243fa65 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -1251,7 +1251,7 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { if err != nil { return 0, 0, err } - defer resp1.Body.Close() + defer resp1.Body.Close() //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp content, totalLines, _, err := buffer.ProcessResponseAsRingBufferToEnd(resp1, bufferSize) return totalLines, int64(len(content)), err }) @@ -1263,7 +1263,7 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { if err != nil { return 0, 0, err } - defer resp2.Body.Close() + defer resp2.Body.Close() //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp content, err := io.ReadAll(resp2.Body) if err != nil { return 0, 0, err diff --git a/pkg/profiler/profiler.go b/pkg/profiler/profiler.go index 1ebc0c2e6..765445592 100644 --- a/pkg/profiler/profiler.go +++ b/pkg/profiler/profiler.go @@ -9,6 +9,7 @@ import ( "time" "log/slog" + "math" ) // Profile represents performance metrics for an operation @@ -35,6 +36,26 @@ func (p *Profile) String() string { ) } +func safeMemoryDelta(after, before uint64) int64 { + if after > math.MaxInt64 || before > math.MaxInt64 { + if after >= before { + diff := after - before + if diff > math.MaxInt64 { + return math.MaxInt64 + } + return int64(diff) + } else { + diff := before - after + if diff > math.MaxInt64 { + return -math.MaxInt64 + } + return -int64(diff) + } + } + + return int64(after) - int64(before) +} + // Profiler provides minimal performance profiling capabilities type Profiler struct { logger *slog.Logger @@ -71,7 +92,7 @@ func (p *Profiler) ProfileFunc(ctx context.Context, operation string, fn func() var memAfter runtime.MemStats runtime.ReadMemStats(&memAfter) profile.MemoryAfter = memAfter.Alloc - profile.MemoryDelta = int64(memAfter.Alloc) - int64(memBefore.Alloc) + profile.MemoryDelta = safeMemoryDelta(memAfter.Alloc, memBefore.Alloc) if p.logger != nil { p.logger.InfoContext(ctx, "Performance profile", "profile", profile.String()) @@ -105,7 +126,7 @@ func (p *Profiler) ProfileFuncWithMetrics(ctx context.Context, operation string, var memAfter runtime.MemStats runtime.ReadMemStats(&memAfter) profile.MemoryAfter = memAfter.Alloc - profile.MemoryDelta = int64(memAfter.Alloc) - int64(memBefore.Alloc) + profile.MemoryDelta = safeMemoryDelta(memAfter.Alloc, memBefore.Alloc) if p.logger != nil { p.logger.InfoContext(ctx, "Performance profile", "profile", profile.String()) @@ -139,7 +160,7 @@ func (p *Profiler) Start(ctx context.Context, operation string) func(lines int, var memAfter runtime.MemStats runtime.ReadMemStats(&memAfter) profile.MemoryAfter = memAfter.Alloc - profile.MemoryDelta = int64(memAfter.Alloc) - int64(memBefore.Alloc) + profile.MemoryDelta = safeMemoryDelta(memAfter.Alloc, memBefore.Alloc) if p.logger != nil { p.logger.InfoContext(ctx, "Performance profile", "profile", profile.String()) From d9c8825775e6c3a8bab1d41d1b643b5cbb0e7308 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:44:49 +0100 Subject: [PATCH 23/40] linter fixes --- pkg/github/actions_test.go | 4 ++-- pkg/profiler/profiler.go | 11 +++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index bd243fa65..33549aad9 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -1251,8 +1251,8 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { if err != nil { return 0, 0, err } - defer resp1.Body.Close() //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp - content, totalLines, _, err := buffer.ProcessResponseAsRingBufferToEnd(resp1, bufferSize) + defer resp1.Body.Close() //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp + content, totalLines, _, err := buffer.ProcessResponseAsRingBufferToEnd(resp1, bufferSize) //nolint:bodyclose return totalLines, int64(len(content)), err }) require.NoError(t, err1) diff --git a/pkg/profiler/profiler.go b/pkg/profiler/profiler.go index 765445592..1cfb7ffae 100644 --- a/pkg/profiler/profiler.go +++ b/pkg/profiler/profiler.go @@ -44,13 +44,12 @@ func safeMemoryDelta(after, before uint64) int64 { return math.MaxInt64 } return int64(diff) - } else { - diff := before - after - if diff > math.MaxInt64 { - return -math.MaxInt64 - } - return -int64(diff) } + diff := before - after + if diff > math.MaxInt64 { + return -math.MaxInt64 + } + return -int64(diff) } return int64(after) - int64(before) From ec070ee1eed20ee63abf47c336324f9cfa579eec Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Fri, 15 Aug 2025 16:53:50 +0100 Subject: [PATCH 24/40] Update pkg/buffer/buffer.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/buffer/buffer.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/buffer/buffer.go b/pkg/buffer/buffer.go index de7882e41..950690104 100644 --- a/pkg/buffer/buffer.go +++ b/pkg/buffer/buffer.go @@ -7,6 +7,22 @@ import ( "strings" ) +// 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. +// +// Parameters: +// httpResp: The HTTP response whose body will be read. +// maxJobLogLines: The maximum number of log lines to retain. +// +// Returns: +// string: The concatenated log lines (up to maxJobLogLines), separated by newlines. +// int: The total number of lines read from the response. +// *http.Response: The original HTTP response. +// error: Any error encountered during reading. +// +// 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. func ProcessResponseAsRingBufferToEnd(httpResp *http.Response, maxJobLogLines int) (string, int, *http.Response, error) { lines := make([]string, maxJobLogLines) validLines := make([]bool, maxJobLogLines) From 0434b7eea0ed2106ca371688812071b478ded9ba Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 10:37:44 +0100 Subject: [PATCH 25/40] use flag for maxJobLogLines --- cmd/github-mcp-server/main.go | 2 ++ pkg/github/actions.go | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index cad002666..0b8960588 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -75,6 +75,7 @@ func init() { rootCmd.PersistentFlags().Bool("enable-command-logging", false, "When enabled, the server will log all command requests and responses to the log file") rootCmd.PersistentFlags().Bool("export-translations", false, "Save translations to a JSON file") rootCmd.PersistentFlags().String("gh-host", "", "Specify the GitHub hostname (for GitHub Enterprise etc.)") + rootCmd.PersistentFlags().Int("content-window-size", 5000, "Specify the content window size") // Bind flag to viper _ = viper.BindPFlag("toolsets", rootCmd.PersistentFlags().Lookup("toolsets")) @@ -84,6 +85,7 @@ func init() { _ = viper.BindPFlag("enable-command-logging", rootCmd.PersistentFlags().Lookup("enable-command-logging")) _ = viper.BindPFlag("export-translations", rootCmd.PersistentFlags().Lookup("export-translations")) _ = viper.BindPFlag("host", rootCmd.PersistentFlags().Lookup("gh-host")) + _ = viper.BindPFlag("content_window_size", rootCmd.PersistentFlags().Lookup("content-window-size")) // Add subcommands rootCmd.AddCommand(stdioCmd) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 855a01b53..c459efd6b 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -8,6 +8,8 @@ import ( "strconv" "strings" + "github.com/spf13/viper" + buffer "github.com/github/github-mcp-server/pkg/buffer" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/profiler" @@ -20,7 +22,6 @@ import ( const ( DescriptionRepositoryOwner = "Repository owner" DescriptionRepositoryName = "Repository name" - maxJobLogLines = 50000 ) // ListWorkflows creates a tool to list workflows in a repository @@ -748,6 +749,11 @@ func downloadLogContent(ctx context.Context, logURL string, tailLines int) (stri prof := profiler.New(nil, profiler.IsProfilingEnabled()) finish := prof.Start(ctx, "log_buffer_processing") + maxJobLogLines := viper.GetInt("content_window_size") + if maxJobLogLines <= 0 || maxJobLogLines > 10000 { + maxJobLogLines = 10000 + } + httpResp, err := http.Get(logURL) //nolint:gosec if err != nil { return "", 0, httpResp, fmt.Errorf("failed to download logs: %w", err) From fb301c66e821786840c496773b9a9def724e7974 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 11:43:25 +0100 Subject: [PATCH 26/40] add param passing for context window size --- cmd/github-mcp-server/generate_docs.go | 4 ++-- cmd/github-mcp-server/main.go | 1 + internal/ghmcp/server.go | 23 ++++++++++++------- pkg/github/actions.go | 31 ++++++++++---------------- pkg/github/actions_test.go | 10 ++++----- pkg/github/tools.go | 4 ++-- 6 files changed, 37 insertions(+), 36 deletions(-) diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 7fc62b1ae..89cc37c22 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -64,7 +64,7 @@ func generateReadmeDocs(readmePath string) error { t, _ := translations.TranslationHelper() // Create toolset group with mock clients - tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t) + tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000) // Generate toolsets documentation toolsetsDoc := generateToolsetsDoc(tsg) @@ -302,7 +302,7 @@ func generateRemoteToolsetsDoc() string { t, _ := translations.TranslationHelper() // Create toolset group with mock clients - tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t) + tsg := github.DefaultToolsetGroup(false, mockGetClient, mockGetGQLClient, mockGetRawClient, t, 5000) // Generate table header buf.WriteString("| Name | Description | API URL | 1-Click Install (VS Code) | Read-only Link | 1-Click Read-only Install (VS Code) |\n") diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 0b8960588..1aa0b4934 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -55,6 +55,7 @@ var ( ExportTranslations: viper.GetBool("export-translations"), EnableCommandLogging: viper.GetBool("enable-command-logging"), LogFilePath: viper.GetString("log-file"), + ContextWindowSize: viper.GetInt("content_window_size"), } return ghmcp.RunStdioServer(stdioServerConfig) }, diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 5fb9582b9..47dba7825 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -47,6 +47,9 @@ type MCPServerConfig struct { // Translator provides translated text for the server tooling Translator translations.TranslationHelperFunc + + // Context window size + ContextWindowSize int } const stdioServerLogPrefix = "stdioserver" @@ -132,7 +135,7 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { } // Create default toolsets - tsg := github.DefaultToolsetGroup(cfg.ReadOnly, getClient, getGQLClient, getRawClient, cfg.Translator) + tsg := github.DefaultToolsetGroup(cfg.ReadOnly, getClient, getGQLClient, getRawClient, cfg.Translator, cfg.ContextWindowSize) err = tsg.EnableToolsets(enabledToolsets) if err != nil { @@ -180,6 +183,9 @@ type StdioServerConfig struct { // Path to the log file if not stderr LogFilePath string + + // Context window size + ContextWindowSize int } // RunStdioServer is not concurrent safe. @@ -191,13 +197,14 @@ func RunStdioServer(cfg StdioServerConfig) error { t, dumpTranslations := translations.TranslationHelper() ghServer, err := NewMCPServer(MCPServerConfig{ - Version: cfg.Version, - Host: cfg.Host, - Token: cfg.Token, - EnabledToolsets: cfg.EnabledToolsets, - DynamicToolsets: cfg.DynamicToolsets, - ReadOnly: cfg.ReadOnly, - Translator: t, + Version: cfg.Version, + Host: cfg.Host, + Token: cfg.Token, + EnabledToolsets: cfg.EnabledToolsets, + DynamicToolsets: cfg.DynamicToolsets, + ReadOnly: cfg.ReadOnly, + Translator: t, + ContextWindowSize: cfg.ContextWindowSize, }) if err != nil { return fmt.Errorf("failed to create MCP server: %w", err) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index c459efd6b..e8a2c8bd9 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -8,8 +8,6 @@ import ( "strconv" "strings" - "github.com/spf13/viper" - buffer "github.com/github/github-mcp-server/pkg/buffer" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/profiler" @@ -533,7 +531,7 @@ func ListWorkflowJobs(getClient GetClientFn, t translations.TranslationHelperFun } // GetJobLogs creates a tool to download logs for a specific workflow job or efficiently get all failed job logs for a workflow run -func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { +func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc, contextWindowSize int) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("get_job_logs", mcp.WithDescription(t("TOOL_GET_JOB_LOGS_DESCRIPTION", "Download logs for a specific workflow job or efficiently get all failed job logs for a workflow run")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ @@ -616,10 +614,10 @@ func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc) (to if failedOnly && runID > 0 { // Handle failed-only mode: get logs for all failed jobs in the workflow run - return handleFailedJobLogs(ctx, client, owner, repo, int64(runID), returnContent, tailLines) + return handleFailedJobLogs(ctx, client, owner, repo, int64(runID), returnContent, tailLines, contextWindowSize) } else if jobID > 0 { // Handle single job mode - return handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), returnContent, tailLines) + return handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), returnContent, tailLines, contextWindowSize) } return mcp.NewToolResultError("Either job_id must be provided for single job logs, or run_id with failed_only=true for failed job logs"), nil @@ -627,7 +625,7 @@ func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc) (to } // handleFailedJobLogs gets logs for all failed jobs in a workflow run -func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, returnContent bool, tailLines int) (*mcp.CallToolResult, error) { +func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, returnContent bool, tailLines int, contextWindowSize int) (*mcp.CallToolResult, error) { // First, get all jobs for the workflow run jobs, resp, err := client.Actions.ListWorkflowJobs(ctx, owner, repo, runID, &github.ListWorkflowJobsOptions{ Filter: "latest", @@ -659,7 +657,7 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo // Collect logs for all failed jobs var logResults []map[string]any for _, job := range failedJobs { - jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent, tailLines) + jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent, tailLines, contextWindowSize) if err != nil { // Continue with other jobs even if one fails jobResult = map[string]any{ @@ -692,8 +690,8 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo } // handleSingleJobLogs gets logs for a single job -func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, returnContent bool, tailLines int) (*mcp.CallToolResult, error) { - jobResult, resp, err := getJobLogData(ctx, client, owner, repo, jobID, "", returnContent, tailLines) +func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, returnContent bool, tailLines int, contextWindowSize int) (*mcp.CallToolResult, error) { + jobResult, resp, err := getJobLogData(ctx, client, owner, repo, jobID, "", returnContent, tailLines, contextWindowSize) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get job logs", resp, err), nil } @@ -707,7 +705,7 @@ func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo } // getJobLogData retrieves log data for a single job, either as URL or content -func getJobLogData(ctx context.Context, client *github.Client, owner, repo string, jobID int64, jobName string, returnContent bool, tailLines int) (map[string]any, *github.Response, error) { +func getJobLogData(ctx context.Context, client *github.Client, owner, repo string, jobID int64, jobName string, returnContent bool, tailLines int, contextWindowSize int) (map[string]any, *github.Response, error) { // Get the download URL for the job logs url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, owner, repo, jobID, 1) if err != nil { @@ -724,7 +722,7 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin if returnContent { // Download and return the actual log content - content, originalLength, httpResp, err := downloadLogContent(ctx, url.String(), tailLines) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp + content, originalLength, httpResp, err := downloadLogContent(ctx, url.String(), tailLines, contextWindowSize) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp if err != nil { // To keep the return value consistent wrap the response as a GitHub Response ghRes := &github.Response{ @@ -745,15 +743,10 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin return result, resp, nil } -func downloadLogContent(ctx context.Context, logURL string, tailLines int) (string, int, *http.Response, error) { +func downloadLogContent(ctx context.Context, logURL string, tailLines int, maxLines int) (string, int, *http.Response, error) { prof := profiler.New(nil, profiler.IsProfilingEnabled()) finish := prof.Start(ctx, "log_buffer_processing") - maxJobLogLines := viper.GetInt("content_window_size") - if maxJobLogLines <= 0 || maxJobLogLines > 10000 { - maxJobLogLines = 10000 - } - httpResp, err := http.Get(logURL) //nolint:gosec if err != nil { return "", 0, httpResp, fmt.Errorf("failed to download logs: %w", err) @@ -769,8 +762,8 @@ func downloadLogContent(ctx context.Context, logURL string, tailLines int) (stri } bufferSize := tailLines - if bufferSize > maxJobLogLines { - bufferSize = maxJobLogLines + if bufferSize > maxLines { + bufferSize = maxLines } processedInput, totalLines, httpResp, err := buffer.ProcessResponseAsRingBufferToEnd(httpResp, bufferSize) diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index 33549aad9..e82aabf62 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -814,7 +814,7 @@ func Test_GetWorkflowRunUsage(t *testing.T) { func Test_GetJobLogs(t *testing.T) { // Verify tool definition once mockClient := github.NewClient(nil) - tool, _ := GetJobLogs(stubGetClientFn(mockClient), translations.NullTranslationHelper) + tool, _ := GetJobLogs(stubGetClientFn(mockClient), translations.NullTranslationHelper, 5000) assert.Equal(t, "get_job_logs", tool.Name) assert.NotEmpty(t, tool.Description) @@ -1043,7 +1043,7 @@ func Test_GetJobLogs(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper) + _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper, 5000) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1102,7 +1102,7 @@ func Test_GetJobLogs_WithContentReturn(t *testing.T) { ) client := github.NewClient(mockedClient) - _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper) + _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper, 5000) request := createMCPRequest(map[string]any{ "owner": "owner", @@ -1149,7 +1149,7 @@ func Test_GetJobLogs_WithContentReturnAndTailLines(t *testing.T) { ) client := github.NewClient(mockedClient) - _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper) + _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper, 5000) request := createMCPRequest(map[string]any{ "owner": "owner", @@ -1196,7 +1196,7 @@ func Test_GetJobLogs_WithContentReturnAndLargeTailLines(t *testing.T) { ) client := github.NewClient(mockedClient) - _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper) + _, handler := GetJobLogs(stubGetClientFn(client), translations.NullTranslationHelper, 5000) request := createMCPRequest(map[string]any{ "owner": "owner", diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 3fb39ada7..c1a6b26d4 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -16,7 +16,7 @@ type GetGQLClientFn func(context.Context) (*githubv4.Client, error) var DefaultTools = []string{"all"} -func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetGQLClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc) *toolsets.ToolsetGroup { +func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetGQLClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc, contextWindowSize int) *toolsets.ToolsetGroup { tsg := toolsets.NewToolsetGroup(readOnly) // Define all available features with their default state (disabled) @@ -146,7 +146,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG toolsets.NewServerTool(GetWorkflowRun(getClient, t)), toolsets.NewServerTool(GetWorkflowRunLogs(getClient, t)), toolsets.NewServerTool(ListWorkflowJobs(getClient, t)), - toolsets.NewServerTool(GetJobLogs(getClient, t)), + toolsets.NewServerTool(GetJobLogs(getClient, t, contextWindowSize)), toolsets.NewServerTool(ListWorkflowRunArtifacts(getClient, t)), toolsets.NewServerTool(DownloadWorkflowRunArtifact(getClient, t)), toolsets.NewServerTool(GetWorkflowRunUsage(getClient, t)), From 106d802a5b78041cd4dcb1e6a01236626e96f2fb Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 12:47:02 +0100 Subject: [PATCH 27/40] refactor: rename contextWindowSize to contentWindowSize for consistency --- cmd/github-mcp-server/main.go | 2 +- internal/ghmcp/server.go | 12 ++++++------ pkg/github/actions.go | 18 +++++++++--------- pkg/github/tools.go | 4 ++-- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 1aa0b4934..4d8aa6715 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -55,7 +55,7 @@ var ( ExportTranslations: viper.GetBool("export-translations"), EnableCommandLogging: viper.GetBool("enable-command-logging"), LogFilePath: viper.GetString("log-file"), - ContextWindowSize: viper.GetInt("content_window_size"), + ContentWindowSize: viper.GetInt("content_window_size"), } return ghmcp.RunStdioServer(stdioServerConfig) }, diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 47dba7825..7ad71532f 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -48,8 +48,8 @@ type MCPServerConfig struct { // Translator provides translated text for the server tooling Translator translations.TranslationHelperFunc - // Context window size - ContextWindowSize int + // Content window size + ContentWindowSize int } const stdioServerLogPrefix = "stdioserver" @@ -135,7 +135,7 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { } // Create default toolsets - tsg := github.DefaultToolsetGroup(cfg.ReadOnly, getClient, getGQLClient, getRawClient, cfg.Translator, cfg.ContextWindowSize) + tsg := github.DefaultToolsetGroup(cfg.ReadOnly, getClient, getGQLClient, getRawClient, cfg.Translator, cfg.ContentWindowSize) err = tsg.EnableToolsets(enabledToolsets) if err != nil { @@ -184,8 +184,8 @@ type StdioServerConfig struct { // Path to the log file if not stderr LogFilePath string - // Context window size - ContextWindowSize int + // Content window size + ContentWindowSize int } // RunStdioServer is not concurrent safe. @@ -204,7 +204,7 @@ func RunStdioServer(cfg StdioServerConfig) error { DynamicToolsets: cfg.DynamicToolsets, ReadOnly: cfg.ReadOnly, Translator: t, - ContextWindowSize: cfg.ContextWindowSize, + ContentWindowSize: cfg.ContentWindowSize, }) if err != nil { return fmt.Errorf("failed to create MCP server: %w", err) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index e8a2c8bd9..d3970ec37 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -531,7 +531,7 @@ func ListWorkflowJobs(getClient GetClientFn, t translations.TranslationHelperFun } // GetJobLogs creates a tool to download logs for a specific workflow job or efficiently get all failed job logs for a workflow run -func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc, contextWindowSize int) (tool mcp.Tool, handler server.ToolHandlerFunc) { +func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc, contentWindowSize int) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("get_job_logs", mcp.WithDescription(t("TOOL_GET_JOB_LOGS_DESCRIPTION", "Download logs for a specific workflow job or efficiently get all failed job logs for a workflow run")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ @@ -614,10 +614,10 @@ func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc, con if failedOnly && runID > 0 { // Handle failed-only mode: get logs for all failed jobs in the workflow run - return handleFailedJobLogs(ctx, client, owner, repo, int64(runID), returnContent, tailLines, contextWindowSize) + return handleFailedJobLogs(ctx, client, owner, repo, int64(runID), returnContent, tailLines, contentWindowSize) } else if jobID > 0 { // Handle single job mode - return handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), returnContent, tailLines, contextWindowSize) + return handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), returnContent, tailLines, contentWindowSize) } return mcp.NewToolResultError("Either job_id must be provided for single job logs, or run_id with failed_only=true for failed job logs"), nil @@ -625,7 +625,7 @@ func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc, con } // handleFailedJobLogs gets logs for all failed jobs in a workflow run -func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, returnContent bool, tailLines int, contextWindowSize int) (*mcp.CallToolResult, error) { +func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, returnContent bool, tailLines int, contentWindowSize int) (*mcp.CallToolResult, error) { // First, get all jobs for the workflow run jobs, resp, err := client.Actions.ListWorkflowJobs(ctx, owner, repo, runID, &github.ListWorkflowJobsOptions{ Filter: "latest", @@ -657,7 +657,7 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo // Collect logs for all failed jobs var logResults []map[string]any for _, job := range failedJobs { - jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent, tailLines, contextWindowSize) + jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent, tailLines, contentWindowSize) if err != nil { // Continue with other jobs even if one fails jobResult = map[string]any{ @@ -690,8 +690,8 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo } // handleSingleJobLogs gets logs for a single job -func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, returnContent bool, tailLines int, contextWindowSize int) (*mcp.CallToolResult, error) { - jobResult, resp, err := getJobLogData(ctx, client, owner, repo, jobID, "", returnContent, tailLines, contextWindowSize) +func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, returnContent bool, tailLines int, contentWindowSize int) (*mcp.CallToolResult, error) { + jobResult, resp, err := getJobLogData(ctx, client, owner, repo, jobID, "", returnContent, tailLines, contentWindowSize) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get job logs", resp, err), nil } @@ -705,7 +705,7 @@ func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo } // getJobLogData retrieves log data for a single job, either as URL or content -func getJobLogData(ctx context.Context, client *github.Client, owner, repo string, jobID int64, jobName string, returnContent bool, tailLines int, contextWindowSize int) (map[string]any, *github.Response, error) { +func getJobLogData(ctx context.Context, client *github.Client, owner, repo string, jobID int64, jobName string, returnContent bool, tailLines int, contentWindowSize int) (map[string]any, *github.Response, error) { // Get the download URL for the job logs url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, owner, repo, jobID, 1) if err != nil { @@ -722,7 +722,7 @@ func getJobLogData(ctx context.Context, client *github.Client, owner, repo strin if returnContent { // Download and return the actual log content - content, originalLength, httpResp, err := downloadLogContent(ctx, url.String(), tailLines, contextWindowSize) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp + content, originalLength, httpResp, err := downloadLogContent(ctx, url.String(), tailLines, contentWindowSize) //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp if err != nil { // To keep the return value consistent wrap the response as a GitHub Response ghRes := &github.Response{ diff --git a/pkg/github/tools.go b/pkg/github/tools.go index c1a6b26d4..b50499650 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -16,7 +16,7 @@ type GetGQLClientFn func(context.Context) (*githubv4.Client, error) var DefaultTools = []string{"all"} -func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetGQLClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc, contextWindowSize int) *toolsets.ToolsetGroup { +func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetGQLClientFn, getRawClient raw.GetRawClientFn, t translations.TranslationHelperFunc, contentWindowSize int) *toolsets.ToolsetGroup { tsg := toolsets.NewToolsetGroup(readOnly) // Define all available features with their default state (disabled) @@ -146,7 +146,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG toolsets.NewServerTool(GetWorkflowRun(getClient, t)), toolsets.NewServerTool(GetWorkflowRunLogs(getClient, t)), toolsets.NewServerTool(ListWorkflowJobs(getClient, t)), - toolsets.NewServerTool(GetJobLogs(getClient, t, contextWindowSize)), + toolsets.NewServerTool(GetJobLogs(getClient, t, contentWindowSize)), toolsets.NewServerTool(ListWorkflowRunArtifacts(getClient, t)), toolsets.NewServerTool(DownloadWorkflowRunArtifact(getClient, t)), toolsets.NewServerTool(GetWorkflowRunUsage(getClient, t)), From 516c0f731cc3706528af427d60fcfeb0df5b4c31 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 13:41:24 +0100 Subject: [PATCH 28/40] fix: use tailLines if bigger but only if <= 5000 --- pkg/github/actions.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index d3970ec37..d18096907 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -761,9 +761,9 @@ func downloadLogContent(ctx context.Context, logURL string, tailLines int, maxLi tailLines = 1000 } - bufferSize := tailLines - if bufferSize > maxLines { - bufferSize = maxLines + bufferSize := maxLines + if tailLines > maxLines && tailLines <= 5000 { + bufferSize = tailLines } processedInput, totalLines, httpResp, err := buffer.ProcessResponseAsRingBufferToEnd(httpResp, bufferSize) From 6c3c31ad419fbbb8ad00ad330b2c1a759fa32831 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 13:43:59 +0100 Subject: [PATCH 29/40] fix: limit tailLines to a maximum of 500 for log content download --- pkg/github/actions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index d18096907..149a9cb3b 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -762,7 +762,7 @@ func downloadLogContent(ctx context.Context, logURL string, tailLines int, maxLi } bufferSize := maxLines - if tailLines > maxLines && tailLines <= 5000 { + if tailLines > maxLines && tailLines <= 500 { bufferSize = tailLines } From 82d4ce2a5721b607d53b47d04234fc7f18b53339 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 14:29:43 +0100 Subject: [PATCH 30/40] Update cmd/github-mcp-server/main.go Co-authored-by: Adam Holt --- cmd/github-mcp-server/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 4d8aa6715..960678ca5 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -86,7 +86,7 @@ func init() { _ = viper.BindPFlag("enable-command-logging", rootCmd.PersistentFlags().Lookup("enable-command-logging")) _ = viper.BindPFlag("export-translations", rootCmd.PersistentFlags().Lookup("export-translations")) _ = viper.BindPFlag("host", rootCmd.PersistentFlags().Lookup("gh-host")) - _ = viper.BindPFlag("content_window_size", rootCmd.PersistentFlags().Lookup("content-window-size")) + _ = viper.BindPFlag("content-window-size", rootCmd.PersistentFlags().Lookup("content-window-size")) // Add subcommands rootCmd.AddCommand(stdioCmd) From e40e289699a01e5cab40cec77d8caeb9cb742e3b Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 14:29:57 +0100 Subject: [PATCH 31/40] Update cmd/github-mcp-server/main.go Co-authored-by: Adam Holt --- cmd/github-mcp-server/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 960678ca5..0a4545835 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -55,7 +55,7 @@ var ( ExportTranslations: viper.GetBool("export-translations"), EnableCommandLogging: viper.GetBool("enable-command-logging"), LogFilePath: viper.GetString("log-file"), - ContentWindowSize: viper.GetInt("content_window_size"), + ContentWindowSize: viper.GetInt("content-window-size"), } return ghmcp.RunStdioServer(stdioServerConfig) }, From 4310db8133586161c2d8b7ec5b9d4d15472bde07 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 14:31:23 +0100 Subject: [PATCH 32/40] move profiler to internal/ --- {pkg => internal}/profiler/profiler.go | 0 pkg/github/actions.go | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename {pkg => internal}/profiler/profiler.go (100%) diff --git a/pkg/profiler/profiler.go b/internal/profiler/profiler.go similarity index 100% rename from pkg/profiler/profiler.go rename to internal/profiler/profiler.go diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 149a9cb3b..79b090990 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -8,9 +8,9 @@ import ( "strconv" "strings" + "github.com/github/github-mcp-server/internal/profiler" buffer "github.com/github/github-mcp-server/pkg/buffer" ghErrors "github.com/github/github-mcp-server/pkg/errors" - "github.com/github/github-mcp-server/pkg/profiler" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v74/github" "github.com/mark3labs/mcp-go/mcp" From e0767fe70e58b4a0d47ec78d8ecbad311923f3b5 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 14:31:42 +0100 Subject: [PATCH 33/40] update actions test with new profiler location --- pkg/github/actions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index e82aabf62..2864aeee0 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -12,8 +12,8 @@ import ( "strings" "testing" + "github.com/github/github-mcp-server/internal/profiler" buffer "github.com/github/github-mcp-server/pkg/buffer" - "github.com/github/github-mcp-server/pkg/profiler" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v74/github" "github.com/migueleliasweb/go-github-mock/src/mock" From 9677c079bb3d425a1afaed52db54f9c3420ee144 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 15:05:20 +0100 Subject: [PATCH 34/40] fix: adjust buffer size limits --- pkg/buffer/buffer.go | 16 +++++++++------- pkg/github/actions.go | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pkg/buffer/buffer.go b/pkg/buffer/buffer.go index 950690104..b10478b9d 100644 --- a/pkg/buffer/buffer.go +++ b/pkg/buffer/buffer.go @@ -12,14 +12,16 @@ import ( // This efficiently retains the most recent lines, overwriting older ones as needed. // // Parameters: -// httpResp: The HTTP response whose body will be read. -// maxJobLogLines: The maximum number of log lines to retain. +// +// httpResp: The HTTP response whose body will be read. +// maxJobLogLines: The maximum number of log lines to retain. // // Returns: -// string: The concatenated log lines (up to maxJobLogLines), separated by newlines. -// int: The total number of lines read from the response. -// *http.Response: The original HTTP response. -// error: Any error encountered during reading. +// +// string: The concatenated log lines (up to maxJobLogLines), separated by newlines. +// int: The total number of lines read from the response. +// *http.Response: The original HTTP response. +// error: Any error encountered during reading. // // 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. @@ -30,7 +32,7 @@ func ProcessResponseAsRingBufferToEnd(httpResp *http.Response, maxJobLogLines in writeIndex := 0 scanner := bufio.NewScanner(httpResp.Body) - scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + scanner.Buffer(make([]byte, 0, 1024), 1024*1024) for scanner.Scan() { line := scanner.Text() diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 79b090990..bf258a823 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -762,7 +762,7 @@ func downloadLogContent(ctx context.Context, logURL string, tailLines int, maxLi } bufferSize := maxLines - if tailLines > maxLines && tailLines <= 500 { + if tailLines > maxLines && tailLines <= 5000 { bufferSize = tailLines } From ed6bc2dde391b2e48f2df6ccf2de7fb4460bc17c Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 15:17:00 +0100 Subject: [PATCH 35/40] make line buffer 1028kb --- pkg/buffer/buffer.go | 2 +- pkg/github/actions.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/buffer/buffer.go b/pkg/buffer/buffer.go index b10478b9d..546b5324c 100644 --- a/pkg/buffer/buffer.go +++ b/pkg/buffer/buffer.go @@ -32,7 +32,7 @@ func ProcessResponseAsRingBufferToEnd(httpResp *http.Response, maxJobLogLines in writeIndex := 0 scanner := bufio.NewScanner(httpResp.Body) - scanner.Buffer(make([]byte, 0, 1024), 1024*1024) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) for scanner.Scan() { line := scanner.Text() diff --git a/pkg/github/actions.go b/pkg/github/actions.go index bf258a823..d3970ec37 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -8,9 +8,9 @@ import ( "strconv" "strings" - "github.com/github/github-mcp-server/internal/profiler" buffer "github.com/github/github-mcp-server/pkg/buffer" ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/profiler" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v74/github" "github.com/mark3labs/mcp-go/mcp" @@ -761,9 +761,9 @@ func downloadLogContent(ctx context.Context, logURL string, tailLines int, maxLi tailLines = 1000 } - bufferSize := maxLines - if tailLines > maxLines && tailLines <= 5000 { - bufferSize = tailLines + bufferSize := tailLines + if bufferSize > maxLines { + bufferSize = maxLines } processedInput, totalLines, httpResp, err := buffer.ProcessResponseAsRingBufferToEnd(httpResp, bufferSize) From 696e5fddfa0b9bcc596ca3dc0cd4caa1584d015e Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 15:17:27 +0100 Subject: [PATCH 36/40] fix mod path --- pkg/github/actions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index d3970ec37..8a86e9ab6 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -8,9 +8,9 @@ import ( "strconv" "strings" + "github.com/github/github-mcp-server/internal/profiler" buffer "github.com/github/github-mcp-server/pkg/buffer" ghErrors "github.com/github/github-mcp-server/pkg/errors" - "github.com/github/github-mcp-server/pkg/profiler" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v74/github" "github.com/mark3labs/mcp-go/mcp" From 10e1995d1275d5690c2b4b378e66af6933982199 Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 15:20:29 +0100 Subject: [PATCH 37/40] change test to use same buffer size as normal use --- pkg/github/actions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index 2864aeee0..b785d9cef 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -1228,7 +1228,7 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { } const logLines = 100000 - const bufferSize = 1000 + const bufferSize = 5000 largeLogContent := strings.Repeat("log line with some content\n", logLines) testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { From 2eb2e16a94bc0be5a522ab0408bc3d499a91489c Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 15:27:01 +0100 Subject: [PATCH 38/40] improve test for non-sliding window implementation to not count empty lines --- pkg/github/actions_test.go | 45 +++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index b785d9cef..6c9e735c3 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -1229,7 +1229,7 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { const logLines = 100000 const bufferSize = 5000 - largeLogContent := strings.Repeat("log line with some content\n", logLines) + largeLogContent := strings.Repeat("log line with some content\n", logLines-1) + "final log line" testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) @@ -1246,6 +1246,10 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { ctx := context.Background() debug.SetGCPercent(-1) + defer debug.SetGCPercent(100) + + runtime.GC() + runtime.GC() profile1, err1 := profiler.ProfileFuncWithMetrics(ctx, "sliding_window", func() (int, int64, error) { resp1, err := http.Get(testServer.URL) if err != nil { @@ -1257,6 +1261,7 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { }) require.NoError(t, err1) + runtime.GC() runtime.GC() profile2, err2 := profiler.ProfileFuncWithMetrics(ctx, "no_window", func() (int, int64, error) { resp2, err := http.Get(testServer.URL) @@ -1264,23 +1269,47 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { return 0, 0, err } defer resp2.Body.Close() //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp - content, err := io.ReadAll(resp2.Body) + + allContent, err := io.ReadAll(resp2.Body) if err != nil { return 0, 0, err } - lines := strings.Split(string(content), "\n") - if len(lines) > bufferSize { - lines = lines[len(lines)-bufferSize:] + + allLines := strings.Split(string(allContent), "\n") + var nonEmptyLines []string + for _, line := range allLines { + if line != "" { + nonEmptyLines = append(nonEmptyLines, line) + } + } + totalLines := len(nonEmptyLines) + + var resultLines []string + if totalLines > bufferSize { + resultLines = nonEmptyLines[totalLines-bufferSize:] + } else { + resultLines = nonEmptyLines } - result := strings.Join(lines, "\n") - return len(strings.Split(string(content), "\n")), int64(len(result)), nil + + result := strings.Join(resultLines, "\n") + return totalLines, int64(len(result)), nil }) require.NoError(t, err2) - debug.SetGCPercent(100) assert.Greater(t, profile2.MemoryDelta, profile1.MemoryDelta, "Sliding window should use less memory than reading all into memory") + assert.Equal(t, profile1.LinesCount, profile2.LinesCount, + "Both approaches should count the same number of input lines") + assert.InDelta(t, profile1.BytesCount, profile2.BytesCount, 100, + "Both approaches should produce similar output sizes (within 100 bytes)") + + memoryReduction := float64(profile2.MemoryDelta-profile1.MemoryDelta) / float64(profile2.MemoryDelta) * 100 + t.Logf("Memory reduction: %.1f%% (%.2f MB vs %.2f MB)", + memoryReduction, + float64(profile2.MemoryDelta)/1024/1024, + float64(profile1.MemoryDelta)/1024/1024) + t.Logf("Sliding window: %s", profile1.String()) t.Logf("No window: %s", profile2.String()) } From 47aaa01367a7a2f013934b59340624452627e4eb Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 15:31:02 +0100 Subject: [PATCH 39/40] make test memory measurement more accurate --- pkg/github/actions_test.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index 6c9e735c3..555ec04cb 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -1240,35 +1240,40 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { os.Setenv("GITHUB_MCP_PROFILING_ENABLED", "true") defer os.Unsetenv("GITHUB_MCP_PROFILING_ENABLED") - // Initialize the global profiler profiler.InitFromEnv(nil) - ctx := context.Background() debug.SetGCPercent(-1) defer debug.SetGCPercent(100) - runtime.GC() - runtime.GC() + for i := 0; i < 3; i++ { + runtime.GC() + } + + var baselineStats runtime.MemStats + runtime.ReadMemStats(&baselineStats) + profile1, err1 := profiler.ProfileFuncWithMetrics(ctx, "sliding_window", func() (int, int64, error) { resp1, err := http.Get(testServer.URL) if err != nil { return 0, 0, err } - defer resp1.Body.Close() //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp + defer resp1.Body.Close() //nolint:bodyclose content, totalLines, _, err := buffer.ProcessResponseAsRingBufferToEnd(resp1, bufferSize) //nolint:bodyclose return totalLines, int64(len(content)), err }) require.NoError(t, err1) - runtime.GC() - runtime.GC() + for i := 0; i < 3; i++ { + runtime.GC() + } + profile2, err2 := profiler.ProfileFuncWithMetrics(ctx, "no_window", func() (int, int64, error) { resp2, err := http.Get(testServer.URL) if err != nil { return 0, 0, err } - defer resp2.Body.Close() //nolint:bodyclose // Response body is closed in downloadLogContent, but we need to return httpResp + defer resp2.Body.Close() //nolint:bodyclose allContent, err := io.ReadAll(resp2.Body) if err != nil { @@ -1310,6 +1315,7 @@ func Test_MemoryUsage_SlidingWindow_vs_NoWindow(t *testing.T) { float64(profile2.MemoryDelta)/1024/1024, float64(profile1.MemoryDelta)/1024/1024) + t.Logf("Baseline: %d bytes", baselineStats.Alloc) t.Logf("Sliding window: %s", profile1.String()) t.Logf("No window: %s", profile2.String()) } From 556a41c71740fef611374d4705022399a1675c3b Mon Sep 17 00:00:00 2001 From: Matt Holloway Date: Mon, 18 Aug 2025 16:55:55 +0100 Subject: [PATCH 40/40] remove impossible conditional --- pkg/github/actions.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 8a86e9ab6..ace9d7288 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -757,10 +757,6 @@ func downloadLogContent(ctx context.Context, logURL string, tailLines int, maxLi return "", 0, httpResp, fmt.Errorf("failed to download logs: HTTP %d", httpResp.StatusCode) } - if tailLines <= 0 { - tailLines = 1000 - } - bufferSize := tailLines if bufferSize > maxLines { bufferSize = maxLines