Skip to content

Commit 8f32fbb

Browse files
he-yufenghe-yufeng
committed
fix: make job logs failed_only a modifier
1 parent 8a48d07 commit 8f32fbb

3 files changed

Lines changed: 218 additions & 33 deletions

File tree

‎pkg/github/actions.go‎

Lines changed: 51 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ const (
4545
actionsMethodDeleteWorkflowRunLogs = "delete_workflow_run_logs"
4646
)
4747

48-
// handleFailedJobLogs gets logs for all failed jobs in a workflow run
49-
func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, returnContent bool, tailLines int, contentWindowSize int) (*mcp.CallToolResult, any, error) {
48+
func handleRunJobLogs(ctx context.Context, client *github.Client, owner, repo string, runID int64, failedOnly bool, returnContent bool, tailLines int, contentWindowSize int) (*mcp.CallToolResult, any, error) {
5049
// First, get all jobs for the workflow run
5150
jobs, resp, err := client.Actions.ListWorkflowJobs(ctx, owner, repo, runID, &github.ListWorkflowJobsOptions{
5251
Filter: "latest",
@@ -56,28 +55,34 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo
5655
}
5756
defer func() { _ = resp.Body.Close() }()
5857

59-
// Filter for failed jobs
60-
var failedJobs []*github.WorkflowJob
58+
// Filter jobs when requested. Otherwise return logs for every job in the run.
59+
var selectedJobs []*github.WorkflowJob
6160
for _, job := range jobs.Jobs {
62-
if job.GetConclusion() == "failure" {
63-
failedJobs = append(failedJobs, job)
61+
if !failedOnly || job.GetConclusion() == "failure" {
62+
selectedJobs = append(selectedJobs, job)
6463
}
6564
}
6665

67-
if len(failedJobs) == 0 {
66+
if len(selectedJobs) == 0 {
67+
message := "No jobs found in this workflow run"
68+
if failedOnly {
69+
message = "No failed jobs found in this workflow run"
70+
}
6871
result := map[string]any{
69-
"message": "No failed jobs found in this workflow run",
70-
"run_id": runID,
71-
"total_jobs": len(jobs.Jobs),
72-
"failed_jobs": 0,
72+
"message": message,
73+
"run_id": runID,
74+
"total_jobs": len(jobs.Jobs),
75+
}
76+
if failedOnly {
77+
result["failed_jobs"] = 0
7378
}
7479
r, _ := json.Marshal(result)
7580
return utils.NewToolResultText(string(r)), nil, nil
7681
}
7782

78-
// Collect logs for all failed jobs
83+
// Collect logs for all selected jobs
7984
var logResults []map[string]any
80-
for _, job := range failedJobs {
85+
for _, job := range selectedJobs {
8186
jobResult, resp, err := getJobLogData(ctx, client, owner, repo, job.GetID(), job.GetName(), returnContent, tailLines, contentWindowSize)
8287
if err != nil {
8388
// Continue with other jobs even if one fails
@@ -93,14 +98,20 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo
9398
logResults = append(logResults, jobResult)
9499
}
95100

101+
message := fmt.Sprintf("Retrieved logs for %d jobs", len(selectedJobs))
102+
if failedOnly {
103+
message = fmt.Sprintf("Retrieved logs for %d failed jobs", len(selectedJobs))
104+
}
96105
result := map[string]any{
97-
"message": fmt.Sprintf("Retrieved logs for %d failed jobs", len(failedJobs)),
106+
"message": message,
98107
"run_id": runID,
99108
"total_jobs": len(jobs.Jobs),
100-
"failed_jobs": len(failedJobs),
101109
"logs": logResults,
102110
"return_format": map[string]bool{"content": returnContent, "urls": !returnContent},
103111
}
112+
if failedOnly {
113+
result["failed_jobs"] = len(selectedJobs)
114+
}
104115

105116
r, err := json.Marshal(result)
106117
if err != nil {
@@ -111,7 +122,19 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo
111122
}
112123

113124
// handleSingleJobLogs gets logs for a single job
114-
func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, returnContent bool, tailLines int, contentWindowSize int) (*mcp.CallToolResult, any, error) {
125+
func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, failedOnly bool, returnContent bool, tailLines int, contentWindowSize int) (*mcp.CallToolResult, any, error) {
126+
if failedOnly {
127+
workflowJob, resp, err := client.Actions.GetWorkflowJobByID(ctx, owner, repo, jobID)
128+
if err != nil {
129+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get workflow job", resp, err), nil, nil
130+
}
131+
defer func() { _ = resp.Body.Close() }()
132+
133+
if workflowJob.GetConclusion() != "failure" {
134+
return utils.NewToolResultError(fmt.Sprintf("job_id %d has conclusion %q, not failure", jobID, workflowJob.GetConclusion())), nil, nil
135+
}
136+
}
137+
115138
jobResult, resp, err := getJobLogData(ctx, client, owner, repo, jobID, "", returnContent, tailLines, contentWindowSize)
116139
if err != nil {
117140
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get job logs", resp, err), nil, nil
@@ -623,8 +646,8 @@ func ActionsGetJobLogs(t translations.TranslationHelperFunc) inventory.ServerToo
623646
mcp.Tool{
624647
Name: "get_job_logs",
625648
Description: t("TOOL_GET_JOB_LOGS_CONSOLIDATED_DESCRIPTION", `Get logs for GitHub Actions workflow jobs.
626-
Use this tool to retrieve logs for a specific job or all failed jobs in a workflow run.
627-
For single job logs, provide job_id. For all failed jobs in a run, provide run_id with failed_only=true.
649+
Use this tool to retrieve logs for a specific job or all jobs in a workflow run.
650+
Provide job_id for one job, or run_id for all jobs in a run. Set failed_only=true to restrict either mode to failed jobs only.
628651
`),
629652
Annotations: &mcp.ToolAnnotations{
630653
Title: t("TOOL_GET_JOB_LOGS_CONSOLIDATED_USER_TITLE", "Get GitHub Actions workflow job logs"),
@@ -643,15 +666,15 @@ For single job logs, provide job_id. For all failed jobs in a run, provide run_i
643666
},
644667
"job_id": {
645668
Type: "number",
646-
Description: "The unique identifier of the workflow job. Required when getting logs for a single job.",
669+
Description: "The unique identifier of the workflow job. Provide either job_id or run_id, not both.",
647670
},
648671
"run_id": {
649672
Type: "number",
650-
Description: "The unique identifier of the workflow run. Required when failed_only is true to get logs for all failed jobs in the run.",
673+
Description: "The unique identifier of the workflow run. Provide either run_id or job_id, not both.",
651674
},
652675
"failed_only": {
653676
Type: "boolean",
654-
Description: "When true, gets logs for all failed jobs in the workflow run specified by run_id. Requires run_id to be provided.",
677+
Description: "When true, only returns logs for failed jobs. With job_id, the job must have failed. With run_id, only failed jobs in the run are included.",
655678
},
656679
"return_content": {
657680
Type: "boolean",
@@ -711,23 +734,18 @@ For single job logs, provide job_id. For all failed jobs in a run, provide run_i
711734
return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err)
712735
}
713736

714-
// Validate parameters
715-
if failedOnly && runID == 0 {
716-
return utils.NewToolResultError("run_id is required when failed_only is true"), nil, nil
737+
if jobID > 0 && runID > 0 {
738+
return utils.NewToolResultError("provide either job_id or run_id, not both"), nil, nil
717739
}
718-
if !failedOnly && jobID == 0 {
719-
return utils.NewToolResultError("job_id is required when failed_only is false"), nil, nil
740+
if jobID == 0 && runID == 0 {
741+
return utils.NewToolResultError("one of job_id or run_id must be provided"), nil, nil
720742
}
721743

722-
if failedOnly && runID > 0 {
723-
// Handle failed-only mode: get logs for all failed jobs in the workflow run
724-
return handleFailedJobLogs(ctx, client, owner, repo, int64(runID), returnContent, tailLines, deps.GetContentWindowSize())
725-
} else if jobID > 0 {
726-
// Handle single job mode
727-
return handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), returnContent, tailLines, deps.GetContentWindowSize())
744+
if runID > 0 {
745+
return handleRunJobLogs(ctx, client, owner, repo, int64(runID), failedOnly, returnContent, tailLines, deps.GetContentWindowSize())
728746
}
729747

730-
return utils.NewToolResultError("Either job_id must be provided for single job logs, or run_id with failed_only=true for failed job logs"), nil, nil
748+
return handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), failedOnly, returnContent, tailLines, deps.GetContentWindowSize())
731749
},
732750
)
733751
return tool

‎pkg/github/actions_test.go‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -581,11 +581,141 @@ func Test_ActionsGetJobLogs_SingleJob(t *testing.T) {
581581
assert.Contains(t, response, "logs_url")
582582
assert.Equal(t, "Job logs are available for download", response["message"])
583583
})
584+
585+
t.Run("successful failed-only single job logs", func(t *testing.T) {
586+
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
587+
GetReposActionsJobsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
588+
job := &github.WorkflowJob{
589+
ID: github.Ptr(int64(123)),
590+
Name: github.Ptr("test-job"),
591+
Conclusion: github.Ptr("failure"),
592+
}
593+
w.WriteHeader(http.StatusOK)
594+
_ = json.NewEncoder(w).Encode(job)
595+
}),
596+
GetReposActionsJobsLogsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
597+
w.Header().Set("Location", "https://github.com/logs/job/123")
598+
w.WriteHeader(http.StatusFound)
599+
}),
600+
})
601+
602+
client := github.NewClient(mockedClient)
603+
deps := BaseDeps{
604+
Client: client,
605+
ContentWindowSize: 5000,
606+
}
607+
handler := toolDef.Handler(deps)
608+
609+
request := createMCPRequest(map[string]any{
610+
"owner": "owner",
611+
"repo": "repo",
612+
"job_id": float64(123),
613+
"failed_only": true,
614+
})
615+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
616+
617+
require.NoError(t, err)
618+
require.False(t, result.IsError)
619+
620+
textContent := getTextResult(t, result)
621+
var response map[string]any
622+
err = json.Unmarshal([]byte(textContent.Text), &response)
623+
require.NoError(t, err)
624+
assert.Equal(t, float64(123), response["job_id"])
625+
assert.Contains(t, response, "logs_url")
626+
})
627+
628+
t.Run("failed-only single job returns error for successful job", func(t *testing.T) {
629+
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
630+
GetReposActionsJobsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
631+
job := &github.WorkflowJob{
632+
ID: github.Ptr(int64(123)),
633+
Name: github.Ptr("test-job"),
634+
Conclusion: github.Ptr("success"),
635+
}
636+
w.WriteHeader(http.StatusOK)
637+
_ = json.NewEncoder(w).Encode(job)
638+
}),
639+
})
640+
641+
client := github.NewClient(mockedClient)
642+
deps := BaseDeps{
643+
Client: client,
644+
ContentWindowSize: 5000,
645+
}
646+
handler := toolDef.Handler(deps)
647+
648+
request := createMCPRequest(map[string]any{
649+
"owner": "owner",
650+
"repo": "repo",
651+
"job_id": float64(123),
652+
"failed_only": true,
653+
})
654+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
655+
656+
require.NoError(t, err)
657+
require.True(t, result.IsError)
658+
assert.Contains(t, getTextResult(t, result).Text, "success")
659+
})
584660
}
585661

586662
func Test_ActionsGetJobLogs_FailedJobs(t *testing.T) {
587663
toolDef := ActionsGetJobLogs(translations.NullTranslationHelper)
588664

665+
t.Run("successful all jobs logs", func(t *testing.T) {
666+
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
667+
GetReposActionsRunsJobsByOwnerByRepoByRunID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
668+
jobs := &github.Jobs{
669+
TotalCount: github.Ptr(2),
670+
Jobs: []*github.WorkflowJob{
671+
{
672+
ID: github.Ptr(int64(1)),
673+
Name: github.Ptr("test-job-1"),
674+
Conclusion: github.Ptr("success"),
675+
},
676+
{
677+
ID: github.Ptr(int64(2)),
678+
Name: github.Ptr("test-job-2"),
679+
Conclusion: github.Ptr("failure"),
680+
},
681+
},
682+
}
683+
w.WriteHeader(http.StatusOK)
684+
_ = json.NewEncoder(w).Encode(jobs)
685+
}),
686+
GetReposActionsJobsLogsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
687+
w.Header().Set("Location", "https://github.com/logs/job/"+r.URL.Path[len(r.URL.Path)-1:])
688+
w.WriteHeader(http.StatusFound)
689+
}),
690+
})
691+
692+
client := github.NewClient(mockedClient)
693+
deps := BaseDeps{
694+
Client: client,
695+
ContentWindowSize: 5000,
696+
}
697+
handler := toolDef.Handler(deps)
698+
699+
request := createMCPRequest(map[string]any{
700+
"owner": "owner",
701+
"repo": "repo",
702+
"run_id": float64(456),
703+
})
704+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
705+
706+
require.NoError(t, err)
707+
require.False(t, result.IsError)
708+
709+
textContent := getTextResult(t, result)
710+
var response map[string]any
711+
err = json.Unmarshal([]byte(textContent.Text), &response)
712+
require.NoError(t, err)
713+
assert.Equal(t, float64(456), response["run_id"])
714+
assert.Equal(t, float64(2), response["total_jobs"])
715+
assert.Len(t, response["logs"], 2)
716+
assert.Contains(t, response["message"], "Retrieved logs for 2 jobs")
717+
})
718+
589719
t.Run("successful failed jobs logs", func(t *testing.T) {
590720
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
591721
GetReposActionsRunsJobsByOwnerByRepoByRunID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
@@ -693,3 +823,39 @@ func Test_ActionsGetJobLogs_FailedJobs(t *testing.T) {
693823
assert.Equal(t, "No failed jobs found in this workflow run", response["message"])
694824
})
695825
}
826+
827+
func Test_ActionsGetJobLogs_Validation(t *testing.T) {
828+
toolDef := ActionsGetJobLogs(translations.NullTranslationHelper)
829+
client := github.NewClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}))
830+
deps := BaseDeps{
831+
Client: client,
832+
ContentWindowSize: 5000,
833+
}
834+
handler := toolDef.Handler(deps)
835+
836+
t.Run("requires one id", func(t *testing.T) {
837+
request := createMCPRequest(map[string]any{
838+
"owner": "owner",
839+
"repo": "repo",
840+
})
841+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
842+
843+
require.NoError(t, err)
844+
require.True(t, result.IsError)
845+
assert.Contains(t, getTextResult(t, result).Text, "one of job_id or run_id")
846+
})
847+
848+
t.Run("rejects both ids", func(t *testing.T) {
849+
request := createMCPRequest(map[string]any{
850+
"owner": "owner",
851+
"repo": "repo",
852+
"job_id": float64(123),
853+
"run_id": float64(456),
854+
})
855+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
856+
857+
require.NoError(t, err)
858+
require.True(t, result.IsError)
859+
assert.Contains(t, getTextResult(t, result).Text, "not both")
860+
})
861+
}

‎pkg/github/helper_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,7 @@ const (
131131
PostReposActionsRunsRerunByOwnerByRepoByRunID = "POST /repos/{owner}/{repo}/actions/runs/{run_id}/rerun"
132132
PostReposActionsRunsRerunFailedJobsByOwnerByRepoByRunID = "POST /repos/{owner}/{repo}/actions/runs/{run_id}/rerun-failed-jobs"
133133
PostReposActionsRunsCancelByOwnerByRepoByRunID = "POST /repos/{owner}/{repo}/actions/runs/{run_id}/cancel"
134+
GetReposActionsJobsByOwnerByRepoByJobID = "GET /repos/{owner}/{repo}/actions/jobs/{job_id}"
134135
GetReposActionsJobsLogsByOwnerByRepoByJobID = "GET /repos/{owner}/{repo}/actions/jobs/{job_id}/logs"
135136
DeleteReposActionsRunsLogsByOwnerByRepoByRunID = "DELETE /repos/{owner}/{repo}/actions/runs/{run_id}/logs"
136137

0 commit comments

Comments
 (0)