Skip to content

Commit 69f3d3f

Browse files
committed
Include merge_commit_sha in the minimal pull request result
pull_request_read (method get) and list_pull_requests return a trimmed MinimalPullRequest built by convertToMinimalPullRequest, which had no merge_commit_sha field, so a merged pull request gave no way to reach the commit the merge produced without a second call (#3235). Add MergeCommitSHA to MinimalPullRequest and populate it, but only once the PR is merged. An open PR's merge_commit_sha is GitHub's ephemeral test-merge ref (refs/pull/N/merge), not a commit on the base branch, so gating on the merged signal (merged_at present, or merged) avoids handing that back as if it were the merge commit. Add merge_commit_sha to listPullRequestsItemFieldEnum so list_pull_requests can select it, and regenerate that tool's snapshot. Add a converter unit test asserting the field is emitted for a merged PR and withheld for an open or closed-unmerged one.
1 parent 8b3538f commit 69f3d3f

3 files changed

Lines changed: 52 additions & 1 deletion

File tree

‎pkg/github/__toolsnaps__/list_pull_requests.snap‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
"state",
3030
"draft",
3131
"merged",
32+
"merge_commit_sha",
3233
"mergeable_state",
3334
"html_url",
3435
"user",

‎pkg/github/minimal_types.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ var listIssuesItemFieldEnum = []any{
4545
// MinimalPullRequest. The body field is the heaviest, so omitting it is the main
4646
// lever for shrinking large result sets.
4747
var listPullRequestsItemFieldEnum = []any{
48-
"number", "title", "body", "state", "draft", "merged", "mergeable_state",
48+
"number", "title", "body", "state", "draft", "merged", "merge_commit_sha", "mergeable_state",
4949
"html_url", "user", "labels", "assignees", "requested_reviewers", "merged_by",
5050
"head", "base", "additions", "deletions", "changed_files", "commits",
5151
"comments", "created_at", "updated_at", "closed_at", "merged_at", "milestone",
@@ -717,6 +717,7 @@ type MinimalPullRequest struct {
717717
State string `json:"state"`
718718
Draft bool `json:"draft"`
719719
Merged bool `json:"merged"`
720+
MergeCommitSHA string `json:"merge_commit_sha,omitempty"`
720721
MergeableState string `json:"mergeable_state,omitempty"`
721722
HTMLURL string `json:"html_url"`
722723
User *MinimalUser `json:"user,omitempty"`
@@ -1111,6 +1112,11 @@ func convertToMinimalPullRequest(pr *github.PullRequest) MinimalPullRequest {
11111112
if pr.MergedAt != nil {
11121113
m.MergedAt = pr.MergedAt.Format(time.RFC3339)
11131114
}
1115+
// merge_commit_sha is a real base-branch commit only once merged; an open PR's
1116+
// is an ephemeral test-merge ref, so omit it there (#3235).
1117+
if pr.GetMerged() || pr.MergedAt != nil {
1118+
m.MergeCommitSHA = pr.GetMergeCommitSHA()
1119+
}
11141120

11151121
for _, label := range pr.Labels {
11161122
if label != nil {

‎pkg/github/pullrequests_test.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,50 @@ func Test_GetPullRequest(t *testing.T) {
180180
}
181181
}
182182

183+
// Test_convertToMinimalPullRequestMergeCommitSHA pins that merge_commit_sha is
184+
// emitted only once a PR is merged, not for an open PR's ephemeral test-merge
185+
// ref (#3235).
186+
func Test_convertToMinimalPullRequestMergeCommitSHA(t *testing.T) {
187+
const sha = "f1e2d3c4b5a6978012345678901234567890abcd"
188+
tests := []struct {
189+
name string
190+
pr *github.PullRequest
191+
want string
192+
}{
193+
{
194+
name: "merged PR exposes the merge commit",
195+
pr: &github.PullRequest{
196+
State: github.Ptr("closed"),
197+
Merged: github.Ptr(true),
198+
MergedAt: &github.Timestamp{Time: time.Now()},
199+
MergeCommitSHA: github.Ptr(sha),
200+
},
201+
want: sha,
202+
},
203+
{
204+
name: "open PR withholds the ephemeral test-merge SHA",
205+
pr: &github.PullRequest{
206+
State: github.Ptr("open"),
207+
MergeCommitSHA: github.Ptr(sha),
208+
},
209+
want: "",
210+
},
211+
{
212+
name: "closed-unmerged PR withholds any test-merge SHA",
213+
pr: &github.PullRequest{
214+
State: github.Ptr("closed"),
215+
MergeCommitSHA: github.Ptr(sha),
216+
},
217+
want: "",
218+
},
219+
}
220+
for _, tc := range tests {
221+
t.Run(tc.name, func(t *testing.T) {
222+
assert.Equal(t, tc.want, convertToMinimalPullRequest(tc.pr).MergeCommitSHA)
223+
})
224+
}
225+
}
226+
183227
func Test_UpdatePullRequest(t *testing.T) {
184228
// Verify tool definition once
185229
serverTool := UpdatePullRequest(translations.NullTranslationHelper)

0 commit comments

Comments
 (0)