Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new "tail_logs" option to the GitHub MCP Server that lets users retrieve only the last few lines of job logs. Key changes include introducing a new tail_lines parameter in the tool request options, modifying both failed and single job log functions to accept this parameter, and updating tests to verify log truncation behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/github/actions_test.go | Added new test cases to cover tail_lines functionality. |
| pkg/github/actions.go | Updated API definitions and helper functions to support tailing logs. |
Comments suppressed due to low confidence (1)
pkg/github/actions.go:622
- [nitpick] Clarify in the documentation that specifying tail_lines as 0 will default to 50 lines rather than returning no lines. This will help users understand the behavior when passing 0.
}
pkg/github/actions_test.go
Outdated
| checkResponse: func(t *testing.T, response map[string]any) { | ||
| assert.Equal(t, float64(456), response["run_id"]) | ||
| assert.Equal(t, float64(3), response["total_jobs"]) | ||
| assert.Equal(t, float64(2), response["failed_jobs"]) |
There was a problem hiding this comment.
The test expects 2 failed jobs while 3 job entries are provided in the mocked response. Verify if the expected failure count is correct or adjust the mock data accordingly.
SamMorrowDrums
left a comment
SamMorrowDrums
left a comment
There was a problem hiding this comment.
This already looks really close. I think we might want to return the total lines anyway, just to contextualize the value, but we can look at that later.
| // Truncate to tail_lines if specified | ||
| if tailLines > 0 { | ||
| lines := strings.Split(logContent, "\n") | ||
| if len(lines) > tailLines { | ||
| lines = lines[len(lines)-tailLines:] | ||
| logContent = strings.Join(lines, "\n") | ||
| } | ||
| } |
There was a problem hiding this comment.
Nerdy, but you could build from reverse parsing the string maybe, for efficiency perhaps?
if tailLines > 0 {
lineCount := 0
lastNewlinePos := len(logContent)
// Count backwards to find the nth newline from the end
for i := len(logContent) - 1; i >= 0 && lineCount < tailLines; i-- {
if logContent[i] == '\n' {
lineCount++
if lineCount == tailLines {
logContent = logContent[i+1:]
break
}
}
}
}
There was a problem hiding this comment.
Could also add an offset from the bottom, so repeat calls could search upwards without getting the same data twice?
There was a problem hiding this comment.
Yeah it's a good call not to split the log content and move from backwards. Regarding lastNewlinePos - I am not sure how that could be used, right now the function keeps no state, I am not sure how adding this variable could help.
pkg/github/actions.go
Outdated
| ), | ||
| mcp.WithNumber("tail_lines", | ||
| mcp.Description("Number of lines to return from the end of the log"), | ||
| mcp.DefaultNumber(50), |
There was a problem hiding this comment.
We can encourage LLM using this option in the description. But I think the default behavior should be return all if tail_lines not specified?
|
Also in my personal test default to 50 seems too low that usually only reach to post-action steps. |
I bumped it to 500, which now might be too high but let's see. We could think of making the search more powerful and return only meaningful log lines, but it's out of scope for this pr. :) |
Yeah I tried quite a few interesting options, but last n lines is better than nothing as we plan to release tomorrow. We can iterate further though. |
| for i := len(content) - 1; i >= 0 && lineCount < tailLines; i-- { | ||
| if content[i] == '\n' { | ||
| lineCount++ | ||
| if lineCount == tailLines { |
There was a problem hiding this comment.
It might be worth a comment here explaining that there isn't an exit condition because we want the total line count.
4 participants
This PR adds a new option to tail logs.
Closes: #608