From 36e0b01f46f37ce97e0fe83946c82882d35526b9 Mon Sep 17 00:00:00 2001 From: King Star Date: Fri, 12 Jun 2026 01:45:21 +0800 Subject: [PATCH] fix: make failed_only a modifier for job logs --- README.md | 6 +- pkg/github/actions.go | 96 ++++++++++------ pkg/github/actions_test.go | 226 +++++++++++++++++++++++++++++++++++++ pkg/github/helper_test.go | 1 + 4 files changed, 291 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index dc063f22ce..3542cff81f 100644 --- a/README.md +++ b/README.md @@ -628,12 +628,12 @@ The following sets of tools are available: - **get_job_logs** - Get GitHub Actions workflow job logs - **Required OAuth Scopes**: `repo` - - `failed_only`: When true, gets logs for all failed jobs in the workflow run specified by run_id. Requires run_id to be provided. (boolean, optional) - - `job_id`: The unique identifier of the workflow job. Required when getting logs for a single job. (number, optional) + - `failed_only`: When true, returns logs only for failed jobs. With run_id, returns failed jobs in the run. With job_id, returns logs only if that job failed. (boolean, optional) + - `job_id`: The unique identifier of the workflow job. Provide either job_id or run_id, not both. (number, optional) - `owner`: Repository owner (string, required) - `repo`: Repository name (string, required) - `return_content`: Returns actual log content instead of URLs (boolean, optional) - - `run_id`: The unique identifier of the workflow run. Required when failed_only is true to get logs for all failed jobs in the run. (number, optional) + - `run_id`: The unique identifier of the workflow run. Provide either run_id or job_id, not both. (number, optional) - `tail_lines`: Number of lines to return from the end of the log (number, optional) diff --git a/pkg/github/actions.go b/pkg/github/actions.go index 9dac877736..2af58072cf 100644 --- a/pkg/github/actions.go +++ b/pkg/github/actions.go @@ -46,8 +46,8 @@ const ( actionsMethodDeleteWorkflowRunLogs = "delete_workflow_run_logs" ) -// 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, contentWindowSize int) (*mcp.CallToolResult, any, error) { +// handleRunJobLogs gets logs for workflow jobs in a run, optionally limited to failed jobs. +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) { // First, get all jobs for the workflow run jobs, resp, err := client.Actions.ListWorkflowJobs(ctx, owner, repo, runID, &github.ListWorkflowJobsOptions{ Filter: "latest", @@ -57,28 +57,37 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo } defer func() { _ = resp.Body.Close() }() - // Filter for failed jobs - var failedJobs []*github.WorkflowJob - for _, job := range jobs.Jobs { - if job.GetConclusion() == "failure" { - failedJobs = append(failedJobs, job) + selectedJobs := jobs.Jobs + if failedOnly { + selectedJobs = nil + for _, job := range jobs.Jobs { + if job.GetConclusion() == "failure" { + selectedJobs = append(selectedJobs, job) + } } } - if len(failedJobs) == 0 { + if len(selectedJobs) == 0 { + message := "No jobs found in this workflow run" + if failedOnly { + message = "No failed jobs found in this workflow run" + } result := map[string]any{ - "message": "No failed jobs found in this workflow run", - "run_id": runID, - "total_jobs": len(jobs.Jobs), - "failed_jobs": 0, + "message": message, + "run_id": runID, + "total_jobs": len(jobs.Jobs), + "returned_jobs": 0, + } + if failedOnly { + result["failed_jobs"] = 0 } r, _ := json.Marshal(result) return utils.NewToolResultText(string(r)), nil, nil } - // Collect logs for all failed jobs + // Collect logs for the selected jobs. var logResults []map[string]any - for _, job := range failedJobs { + for _, job := range selectedJobs { 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 @@ -94,14 +103,21 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo logResults = append(logResults, jobResult) } + message := fmt.Sprintf("Retrieved logs for %d jobs", len(selectedJobs)) + if failedOnly { + message = fmt.Sprintf("Retrieved logs for %d failed jobs", len(selectedJobs)) + } result := map[string]any{ - "message": fmt.Sprintf("Retrieved logs for %d failed jobs", len(failedJobs)), + "message": message, "run_id": runID, "total_jobs": len(jobs.Jobs), - "failed_jobs": len(failedJobs), + "returned_jobs": len(selectedJobs), "logs": logResults, "return_format": map[string]bool{"content": returnContent, "urls": !returnContent}, } + if failedOnly { + result["failed_jobs"] = len(selectedJobs) + } r, err := json.Marshal(result) if err != nil { @@ -111,8 +127,20 @@ func handleFailedJobLogs(ctx context.Context, client *github.Client, owner, repo return utils.NewToolResultText(string(r)), nil, nil } -// handleSingleJobLogs gets logs for a single job -func handleSingleJobLogs(ctx context.Context, client *github.Client, owner, repo string, jobID int64, returnContent bool, tailLines int, contentWindowSize int) (*mcp.CallToolResult, any, error) { +// handleSingleJobLogs gets logs for a single job, optionally requiring a failed conclusion. +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) { + if failedOnly { + job, resp, err := client.Actions.GetWorkflowJobByID(ctx, owner, repo, jobID) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get workflow job", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + if job.GetConclusion() != "failure" { + return utils.NewToolResultError(fmt.Sprintf("job %d did not fail; conclusion is %s", jobID, job.GetConclusion())), nil, nil + } + } + 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, nil @@ -650,8 +678,8 @@ func ActionsGetJobLogs(t translations.TranslationHelperFunc) inventory.ServerToo mcp.Tool{ Name: "get_job_logs", Description: t("TOOL_GET_JOB_LOGS_CONSOLIDATED_DESCRIPTION", `Get logs for GitHub Actions workflow jobs. -Use this tool to retrieve logs for a specific job or all failed jobs in a workflow run. -For single job logs, provide job_id. For all failed jobs in a run, provide run_id with failed_only=true. +Use this tool to retrieve logs for a specific job or jobs in a workflow run. +Provide job_id for one job, run_id for all jobs in a run, or run_id with failed_only=true for failed jobs only. `), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_GET_JOB_LOGS_CONSOLIDATED_USER_TITLE", "Get GitHub Actions workflow job logs"), @@ -670,15 +698,15 @@ For single job logs, provide job_id. For all failed jobs in a run, provide run_i }, "job_id": { Type: "number", - Description: "The unique identifier of the workflow job. Required when getting logs for a single job.", + Description: "The unique identifier of the workflow job. Provide either job_id or run_id, not both.", }, "run_id": { Type: "number", - Description: "The unique identifier of the workflow run. Required when failed_only is true to get logs for all failed jobs in the run.", + Description: "The unique identifier of the workflow run. Provide either run_id or job_id, not both.", }, "failed_only": { Type: "boolean", - Description: "When true, gets logs for all failed jobs in the workflow run specified by run_id. Requires run_id to be provided.", + Description: "When true, returns logs only for failed jobs. With run_id, returns failed jobs in the run. With job_id, returns logs only if that job failed.", }, "return_content": { Type: "boolean", @@ -739,11 +767,8 @@ For single job logs, provide job_id. For all failed jobs in a run, provide run_i } // Validate parameters - if failedOnly && runID == 0 { - return utils.NewToolResultError("run_id is required when failed_only is true"), nil, nil - } - if !failedOnly && jobID == 0 { - return utils.NewToolResultError("job_id is required when failed_only is false"), nil, nil + if jobID > 0 && runID > 0 { + return utils.NewToolResultError("provide either job_id or run_id, not both"), nil, nil } // attachIFC adds the IFC label to a successful result when IFC @@ -754,17 +779,18 @@ For single job logs, provide job_id. For all failed jobs in a run, provide run_i return attachRepoVisibilityIFCLabel(ctx, deps, client, owner, repo, r, ifc.LabelActionsResult) } - if failedOnly && runID > 0 { - // Handle failed-only mode: get logs for all failed jobs in the workflow run - result, payload, err := handleFailedJobLogs(ctx, client, owner, repo, int64(runID), returnContent, tailLines, deps.GetContentWindowSize()) + if runID > 0 { + // Handle run mode: get logs for all jobs, optionally filtered to failed jobs. + result, payload, err := handleRunJobLogs(ctx, client, owner, repo, int64(runID), failedOnly, returnContent, tailLines, deps.GetContentWindowSize()) return attachIFC(result), payload, err - } else if jobID > 0 { - // Handle single job mode - result, payload, err := handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), returnContent, tailLines, deps.GetContentWindowSize()) + } + if jobID > 0 { + // Handle single job mode. + result, payload, err := handleSingleJobLogs(ctx, client, owner, repo, int64(jobID), failedOnly, returnContent, tailLines, deps.GetContentWindowSize()) return attachIFC(result), payload, err } - 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 + return utils.NewToolResultError("Either job_id or run_id must be provided"), nil, nil }, ) return tool diff --git a/pkg/github/actions_test.go b/pkg/github/actions_test.go index 371bbbe9dc..e70392b769 100644 --- a/pkg/github/actions_test.go +++ b/pkg/github/actions_test.go @@ -581,11 +581,237 @@ func Test_ActionsGetJobLogs_SingleJob(t *testing.T) { assert.Contains(t, response, "logs_url") assert.Equal(t, "Job logs are available for download", response["message"]) }) + + t.Run("failed_only single failed job returns logs", func(t *testing.T) { + mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposActionsJobsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + job := &github.WorkflowJob{ + ID: github.Ptr(int64(123)), + Name: github.Ptr("test-job"), + Conclusion: github.Ptr("failure"), + } + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(job) + }), + GetReposActionsJobsLogsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", "https://github.com/logs/job/123") + w.WriteHeader(http.StatusFound) + }), + }) + + client := mustNewGHClient(t, mockedClient) + deps := BaseDeps{ + Client: client, + ContentWindowSize: 5000, + } + handler := toolDef.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "job_id": float64(123), + "failed_only": true, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &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, "https://github.com/logs/job/123", response["logs_url"]) + }) + + t.Run("failed_only single job skips successful job", func(t *testing.T) { + mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposActionsJobsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + job := &github.WorkflowJob{ + ID: github.Ptr(int64(123)), + Name: github.Ptr("test-job"), + Conclusion: github.Ptr("success"), + } + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(job) + }), + GetReposActionsJobsLogsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Fatal("logs endpoint should not be called when failed_only=true and the job succeeded") + }), + }) + + client := mustNewGHClient(t, mockedClient) + deps := BaseDeps{ + Client: client, + ContentWindowSize: 5000, + } + handler := toolDef.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "job_id": float64(123), + "failed_only": true, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + require.True(t, result.IsError) + + textContent := getTextResult(t, result) + assert.Equal(t, "job 123 did not fail; conclusion is success", textContent.Text) + }) + + t.Run("rejects both job_id and run_id", func(t *testing.T) { + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + deps := BaseDeps{ + Client: client, + } + handler := toolDef.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "job_id": float64(123), + "run_id": float64(456), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + require.True(t, result.IsError) + + textContent := getTextResult(t, result) + assert.Equal(t, "provide either job_id or run_id, not both", textContent.Text) + }) + + t.Run("rejects missing job_id and run_id", func(t *testing.T) { + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + deps := BaseDeps{ + Client: client, + } + handler := toolDef.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + require.True(t, result.IsError) + + textContent := getTextResult(t, result) + assert.Equal(t, "Either job_id or run_id must be provided", textContent.Text) + }) } func Test_ActionsGetJobLogs_FailedJobs(t *testing.T) { toolDef := ActionsGetJobLogs(translations.NullTranslationHelper) + t.Run("successful all job logs for run", func(t *testing.T) { + mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposActionsRunsJobsByOwnerByRepoByRunID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + jobs := &github.Jobs{ + TotalCount: github.Ptr(2), + Jobs: []*github.WorkflowJob{ + { + ID: github.Ptr(int64(1)), + Name: github.Ptr("test-job-1"), + Conclusion: github.Ptr("success"), + }, + { + ID: github.Ptr(int64(2)), + Name: github.Ptr("test-job-2"), + Conclusion: github.Ptr("failure"), + }, + }, + } + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(jobs) + }), + GetReposActionsJobsLogsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", "https://github.com/logs/job/"+r.URL.Path[len(r.URL.Path)-1:]) + w.WriteHeader(http.StatusFound) + }), + }) + + client := mustNewGHClient(t, mockedClient) + deps := BaseDeps{ + Client: client, + ContentWindowSize: 5000, + } + handler := toolDef.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "run_id": float64(456), + "failed_only": false, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &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(456), response["run_id"]) + assert.Equal(t, float64(2), response["total_jobs"]) + assert.Equal(t, float64(2), response["returned_jobs"]) + assert.Contains(t, response["message"], "Retrieved logs for 2 jobs") + }) + + t.Run("successful all job logs for run when failed_only omitted", func(t *testing.T) { + mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposActionsRunsJobsByOwnerByRepoByRunID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + jobs := &github.Jobs{ + TotalCount: github.Ptr(1), + Jobs: []*github.WorkflowJob{ + { + ID: github.Ptr(int64(1)), + Name: github.Ptr("test-job-1"), + Conclusion: github.Ptr("success"), + }, + }, + } + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(jobs) + }), + GetReposActionsJobsLogsByOwnerByRepoByJobID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", "https://github.com/logs/job/1") + w.WriteHeader(http.StatusFound) + }), + }) + + client := mustNewGHClient(t, mockedClient) + deps := BaseDeps{ + Client: client, + ContentWindowSize: 5000, + } + handler := toolDef.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "run_id": float64(456), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &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(456), response["run_id"]) + assert.Equal(t, float64(1), response["returned_jobs"]) + assert.Contains(t, response["message"], "Retrieved logs for 1 jobs") + }) + t.Run("successful failed jobs logs", func(t *testing.T) { mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ GetReposActionsRunsJobsByOwnerByRepoByRunID: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 7f86c8b989..63187fea5d 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -133,6 +133,7 @@ const ( PostReposActionsRunsRerunByOwnerByRepoByRunID = "POST /repos/{owner}/{repo}/actions/runs/{run_id}/rerun" PostReposActionsRunsRerunFailedJobsByOwnerByRepoByRunID = "POST /repos/{owner}/{repo}/actions/runs/{run_id}/rerun-failed-jobs" PostReposActionsRunsCancelByOwnerByRepoByRunID = "POST /repos/{owner}/{repo}/actions/runs/{run_id}/cancel" + GetReposActionsJobsByOwnerByRepoByJobID = "GET /repos/{owner}/{repo}/actions/jobs/{job_id}" GetReposActionsJobsLogsByOwnerByRepoByJobID = "GET /repos/{owner}/{repo}/actions/jobs/{job_id}/logs" DeleteReposActionsRunsLogsByOwnerByRepoByRunID = "DELETE /repos/{owner}/{repo}/actions/runs/{run_id}/logs"