fix: make job logs failed_only a modifier

[he-yufeng]

Summary

• allow get_job_logs to fetch all jobs in a run when run_id is provided without failed_only
• make failed_only a modifier for both run_id and job_id
• reject ambiguous calls that provide both job_id and run_id

Fixes #2389.

To verify

• go test ./pkg/github -run 'Test_ActionsGetJobLogs' -count=1
• go test ./pkg/github -count=1
• go test ./... -count=1
• git diff --check

[FuzzysTodd]

@FuzzysTodd become the expert read all documents and reason the final soulful solution spirit to set it and forget it repaired

…

On Fri, May 15, 2026 at 4:35 PM Yufeng He ***@***.***> wrote: Summary - allow get_job_logs to fetch all jobs in a run when run_id is provided without failed_only - make failed_only a modifier for both run_id and job_id - reject ambiguous calls that provide both job_id and run_id Fixes #2389 <#2389>. To verify - go test ./pkg/github -run 'Test_ActionsGetJobLogs' -count=1 - go test ./pkg/github -count=1 - go test ./... -count=1 - git diff --check ------------------------------ You can view, comment on, or merge this pull request online at: #2482 Commit Summary - 8f32fbb <8f32fbb> fix: make job logs failed_only a modifier File Changes (3 files <https://github.com/github/github-mcp-server/pull/2482/files>) - *M* pkg/github/actions.go <https://github.com/github/github-mcp-server/pull/2482/files#diff-200ac9ef711a21b6316fe07fac3ebe37ccd7b09d3ea254c8ebd6c2bfa43ecaec> (84) - *M* pkg/github/actions_test.go <https://github.com/github/github-mcp-server/pull/2482/files#diff-8fed0935b687a7177039f6b181b4bb797ff26a06edf40c21999059fdcb5f5453> (166) - *M* pkg/github/helper_test.go <https://github.com/github/github-mcp-server/pull/2482/files#diff-e8fa532ff8ba951d7e64e769647b469b857271a3926192545c2d9ab7bb94f402> (1) Patch Links: - https://github.com/github/github-mcp-server/pull/2482.patch - https://github.com/github/github-mcp-server/pull/2482.diff — Reply to this email directly, view it on GitHub <#2482>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BFSEEBQJDEPUTTSTUAXE7AT4255P5AVCNFSM6AAAAACY76RB7OVHI2DSMVQWIX3LMV43ASLTON2WKOZUGQ2TMNZZGU4DANY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[jluocsa]

Nice cleanup of the get_job_logs parameter semantics — making failed_only a true modifier (instead of a mode selector implicitly tied to run_id) reads much more naturally from the agent side, and the validation error messages are now actually helpful. The new tests for the failed-only-on-job_id path and the both-ids/neither-id validation cases are exactly the right coverage.

A few things worth a look before merge:

1. The tool schema snapshot doesn't appear to be updated

Three things changed in the tool definition:

• Top-level Description string
• job_id parameter description
• run_id parameter description
• failed_only parameter description

…but the changed-files list (pkg/github/actions.go, pkg/github/actions_test.go, pkg/github/helper_test.go) doesn't include anything under pkg/github/__toolsnaps__/. Per the repo's snapshot-validation system, that should fail go test ./pkg/github with a diff against __toolsnaps__/get_job_logs.snap (or whatever the file is named for this tool).

The fix is just:

UPDATE_TOOLSNAPS=true go test ./pkg/github -run Test_ActionsGetJobLogs -count=1

…and committing the resulting .snap change. That also serves as a nice reviewable record of the public-surface change.

2. Subtle behavior change for result["failed_jobs"] clients

Before this PR, the run_id path always returned failed_jobs in the response map. After the PR, failed_jobs is only present when failed_only=true:

if failedOnly {
    result["failed_jobs"] = len(selectedJobs)
}

That's a sensible shape change (it'd be weird to report failed_jobs when the user didn't ask to filter by failure), but it's still an output-schema break for any caller that was unconditionally reading that key on the run-level result. Worth a one-liner in the PR body / changelog so downstream users notice.

If you'd prefer to keep the key for backwards compatibility, you could always emit it and just compute it independently:

failedJobsCount := 0
for _, job := range jobs.Jobs {
    if job.GetConclusion() == "failure" {
        failedJobsCount++
    }
}
result["failed_jobs"] = failedJobsCount

…but I don't feel strongly — the "only when asked" shape is arguably cleaner.

3. Previously-tolerated combination now errors

Before: passing both job_id and run_id with failed_only=true silently went down the failed-jobs-in-run path (because the old branch checked failedOnly && runID > 0 first). After: that combination now returns a hard error ("provide either job_id or run_id, not both").

This is the right behavior — that input was always ambiguous — but it is a strict-mode change for any agent that was passing extra fields and relying on the old precedence rule. Probably worth one bullet in the PR description acknowledging it. (Not a blocker.)

4. Tiny nit on handleSingleJobLogs resource handling

if failedOnly {
    workflowJob, 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() }()
    ...
}

The defer is inside the if failedOnly block, so it only schedules the close when we entered that branch — good. But the second getJobLogData call below also returns a resp; if we made it through the failed-only check, we still close that body via defer later in the function. Just double-check the existing close on the lower resp in handleSingleJobLogs still fires for the non-failedOnly path. The diff didn't touch it, so this is probably fine, but it'd be reassuring to confirm via go vet / a quick read.

5. Test for failed_only=true with run_id returning an empty selection

There's "no failed jobs found" coverage for the run-level branch, and the new test exercises the job_id+failed_only=true+success-conclusion error path. Consider one more test: run_id + failed_only=true where all jobs failed (currently only the partial-failure case is exercised in the existing test). Small, but exercises the message construction that was reshuffled.

---

Overall the change is straightforward and the API ergonomics improvement is well worth it. Mostly just want to make sure the toolsnap file is regenerated so the schema change is captured in the diff.