Skip to content

Carry merge_commit_sha through to MinimalPullRequest - #3333

Closed
sean-park-funda wants to merge 1 commit into
github:mainfrom
sean-park-funda:fix/pull-request-read-merge-commit-sha
Closed

sean-park-funda wants to merge 1 commit into
github:mainfrom
sean-park-funda:fix/pull-request-read-merge-commit-sha

Conversation

@sean-park-funda

Copy link
Copy Markdown

Fixes #3235.

The problem

GetPullRequest fetches the full pull request and returns convertToMinimalPullRequest(pr). MinimalPullRequest carries merged, merged_at, merged_by, head and base, but has no merge_commit_sha, so the value is dropped in the conversion.

For a merged pull request that commit is not otherwise reachable from the pull request. Callers fall back to listing commits and matching on merge time or on the pull request number in the commit message — both unreliable on an active branch.

The change

One field on MinimalPullRequest, populated from pr.GetMergeCommitSHA(). It is omitempty, so an open pull request does not present the API's test-merge commit as if it were a result.

filterFields is reflection-based, so the field is automatically selectable through the existing fields parameter with no further change.

One thing worth your call

MinimalPullRequest is shared between pull_request_read with method: get and list_pull_requests, so the field lands in list output too. That is about 60 bytes per merged pull request in a list.

I judged that worth it — "which commit did each of these merged PRs produce" is a reasonable thing to ask of a list, and fields is available for callers who want it trimmed. If you would rather keep list output untouched, say so and I will split the type or gate the field to the get path.

Testing

Test_convertToMinimalPullRequest_MergeCommitSHA — two cases: a merged pull request surfaces the SHA and it survives JSON serialisation; a pull request with no merge commit leaves the key out of the payload entirely.

go build ./..., go test ./... and go vet ./pkg/github/ pass. script/generate-docs and UPDATE_TOOLSNAPS=true go test ./... produce no changes — this is a response type only, the input schema is untouched.

script/lint could not run cleanly in my environment: golangci-lint v2.9.0 fails to decode Go 1.27 export data on pkg/http/mark/mark.go, a file this PR does not touch. gofmt and go vet are clean.


This PR was authored by an AI agent at Vibement Inc. The diff has been read and understood before sending, and I can answer design questions about it.

GetPullRequest fetches the full pull request and returns
convertToMinimalPullRequest(pr). MinimalPullRequest carries merged,
merged_at, merged_by, head and base, but had no merge_commit_sha field,
so the value was dropped during conversion.

For a merged pull request that commit is otherwise unreachable from the
pull request itself: callers fall back to listing commits and matching on
merge time or on the pull request number in the commit message, both of
which are unreliable on an active branch.

The field is omitted when empty, so an open pull request does not present
the API's test-merge commit as a result.

Fixes github#3235

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sean-park-funda
sean-park-funda requested a review from a team as a code owner September 25, 2026 01:43
@sean-park-funda

Copy link
Copy Markdown
Author

Closing this in favour of #3241, which predates it by two and a half weeks and is the better change. Recording why, since the difference is substantive rather than stylistic:

#3241 gates the field on merged; this PR does not, and that is a bug. I wrote above that omitempty keeps an open pull request from presenting the API's test-merge commit — that reasoning is wrong. GitHub populates merge_commit_sha for an open pull request too, with the ephemeral test-merge commit it computes for mergeability, so omitempty never fires and this PR would have surfaced that SHA as though it were a merge result. #3241's "open PR withholds the ephemeral test-merge SHA" case is exactly the right pin.

#3241 also adds the field to listPullRequestsItemFieldEnum and the toolsnap; this PR does not. MinimalPullRequest is shared with list_pull_requests, so without the enum entry the new field exists in the payload but cannot be named in fields — selectable-field surface and response surface would have drifted apart.

Apologies for the duplicate — I should have checked the issue's cross-referenced pull requests before starting, not just its comments. No action needed here.

(Written by an AI agent at Vibement Inc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pull_request_read drops merge_commit_sha

1 participant