github/github-mcp-serverPublic

NotificationsYou must be signed in to change notification settings
Fork1.4k
Star17.7k

Add tail logs option#615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

JoannaaKL merged 6 commits intogithub:mainfromJoannaaKL:tructate-job-logs

Jun 30, 2025

Merged

Add tail logs option#615

JoannaaKL merged 6 commits intogithub:mainfromJoannaaKL:tructate-job-logs

Jun 30, 2025

Conversation

Copy link

Contributor

JoannaaKL commentedJun 30, 2025•
edited
Loading

This PR adds a new option to tail logs.
Closes:#608

Add tail logs option

7d06c88

CopilotAI review requested due to automatic review settings

June 30, 2025 15:03

JoannaaKL requested a review froma team as acode owner

June 30, 2025 15:03

CopilotAI reviewed

Jun 30, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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"])

Copy link

Preview

CopilotAIJun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copilot uses AI. Check for mistakes.

Copy link

EbaooneJul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Remove test

ec50b5e

SamMorrowDrums reviewed

Jun 30, 2025

View reviewed changes

Copy link

Collaborator

SamMorrowDrums left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

pkg/github/actions.go Outdated

Comment on lines 791 to 798

		// 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")
		}
		}

Copy link

Collaborator

SamMorrowDrumsJun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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            }        }    }}

Copy link

Collaborator

SamMorrowDrumsJun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could also add an offset from the bottom, so repeat calls could search upwards without getting the same data twice?

Copy link

ContributorAuthor

JoannaaKLJun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah it's a good call not to split the log content and move from backwards. RegardinglastNewlinePos - 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.

Address pr feedback

36236f5

kehao95 reviewed

Jun 30, 2025

View reviewed changes

pkg/github/actions.go Outdated

		@@ -584,6 +584,10 @@ func GetJobLogs(getClient GetClientFn, t translations.TranslationHelperFunc) (to
		mcp.WithBoolean("return_content",
		mcp.Description("Returns actual log content instead of URLs"),
		),
		mcp.WithNumber("tail_lines",
		mcp.Description("Number of lines to return from the end of the log"),
		mcp.DefaultNumber(50),

Copy link

kehao95Jun 30, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We can encourage LLM using this option in the description. But I think the default behavior should be return all iftail_lines not specified?

Copy link

kehao95 commentedJun 30, 2025•
edited
Loading

Also in my personal test default to 50 seems too low that usually only reach to post-action steps.
Is there better way that we can return only the tail log from failed step? ( might not be the scope of this issue though)

JoannaaKL added3 commits

June 30, 2025 19:36

Return total number of lines

0867d89

Use int

305391c

Return last 500 lines by default

2573620

Copy link

ContributorAuthor

JoannaaKL commentedJun 30, 2025

Also in my personal test default to 50 seems too low that usually only reach to post-action steps. Is there better way that we can return only the tail log from failed step? ( might not be the scope of this issue though)

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. :)

Copy link

Collaborator

SamMorrowDrums commentedJun 30, 2025

Also in my personal test default to 50 seems too low that usually only reach to post-action steps.
Is there better way that we can return only the tail log from failed step? ( might not be the scope of this issue though)

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.