Skip to content

Commit b8f107b

Browse files
jcosta1970SamMorrowDrumsCopilot
authored
Add expectedHeadSha pinning to merge_pull_request (#3182)
* Add head SHA pinning to merge_pull_request * Update merge_pull_request generated documentation * test(pull-requests): harden merge SHA pinning Make the schema and unpinned compatibility coverage explicit, and verify stale-head conflicts surface without a retry. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Sam Morrow <sammorrowdrums@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent ffa16d0 commit b8f107b

5 files changed

Lines changed: 110 additions & 2 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,6 +1182,7 @@ The following sets of tools are available:
11821182
- **OAuth Challenge Scopes**: `repo`
11831183
- `commit_message`: Extra detail for merge commit (string, optional)
11841184
- `commit_title`: Title for merge commit (string, optional)
1185+
- `expectedHeadSha`: The expected SHA of the pull request's HEAD ref (string, optional)
11851186
- `merge_method`: Merge method (string, optional)
11861187
- `owner`: Repository owner (string, required)
11871188
- `pullNumber`: Pull request number (number, required)

‎pkg/github/__toolsnaps__/merge_pull_request.snap‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@
2727
"description": "Title for merge commit",
2828
"type": "string"
2929
},
30+
"expectedHeadSha": {
31+
"description": "The expected SHA of the pull request's HEAD ref",
32+
"type": "string"
33+
},
3034
"merge_method": {
3135
"description": "Merge method",
3236
"enum": [
@@ -57,4 +61,4 @@
5761
"type": "object"
5862
},
5963
"name": "merge_pull_request"
60-
}
64+
}

‎pkg/github/pullrequests.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1513,6 +1513,10 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool
15131513
Description: "Merge method",
15141514
Enum: []any{"merge", "squash", "rebase"},
15151515
},
1516+
"expectedHeadSha": {
1517+
Type: "string",
1518+
Description: "The expected SHA of the pull request's HEAD ref",
1519+
},
15161520
},
15171521
Required: []string{"owner", "repo", "pullNumber"},
15181522
}
@@ -1555,9 +1559,14 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool
15551559
if err != nil {
15561560
return utils.NewToolResultError(err.Error()), nil, nil
15571561
}
1562+
expectedHeadSHA, err := OptionalParam[string](args, "expectedHeadSha")
1563+
if err != nil {
1564+
return utils.NewToolResultError(err.Error()), nil, nil
1565+
}
15581566

15591567
options := &github.PullRequestOptions{
15601568
CommitTitle: commitTitle,
1569+
SHA: expectedHeadSHA,
15611570
MergeMethod: mergeMethod,
15621571
}
15631572

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package github
2+
3+
import (
4+
"context"
5+
"net/http"
6+
"testing"
7+
8+
"github.com/github/github-mcp-server/pkg/translations"
9+
"github.com/google/go-github/v89/github"
10+
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
func Test_MergePullRequestSHAPinning(t *testing.T) {
15+
serverTool := MergePullRequest(translations.NullTranslationHelper)
16+
17+
const pinnedSHA = "0123456789abcdef0123456789abcdef01234567"
18+
19+
mockMergeResult := &github.PullRequestMergeResult{
20+
SHA: github.Ptr("merge-result-sha"),
21+
Merged: github.Ptr(true),
22+
Message: github.Ptr("Pull Request successfully merged"),
23+
}
24+
25+
t.Run("expected head SHA is sent as REST sha", func(t *testing.T) {
26+
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
27+
PutReposPullsMergeByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{
28+
"merge_method": "merge",
29+
"sha": pinnedSHA,
30+
}).andThen(
31+
mockResponse(t, http.StatusOK, mockMergeResult),
32+
),
33+
}))
34+
35+
deps := BaseDeps{Client: client}
36+
handler := serverTool.Handler(deps)
37+
38+
request := createMCPRequest(map[string]any{
39+
"owner": "owner",
40+
"repo": "repo",
41+
"pullNumber": float64(42),
42+
"merge_method": "merge",
43+
"expectedHeadSha": pinnedSHA,
44+
})
45+
46+
result, err := handler(
47+
ContextWithDeps(context.Background(), deps),
48+
&request,
49+
)
50+
51+
require.NoError(t, err)
52+
require.False(t, result.IsError)
53+
})
54+
55+
t.Run("head mismatch fails closed without retry", func(t *testing.T) {
56+
calls := 0
57+
58+
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
59+
PutReposPullsMergeByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{
60+
"merge_method": "merge",
61+
"sha": pinnedSHA,
62+
}).andThen(func(w http.ResponseWriter, _ *http.Request) {
63+
calls++
64+
w.Header().Set("Content-Type", "application/json")
65+
w.WriteHeader(http.StatusConflict)
66+
_, _ = w.Write([]byte(
67+
`{"message":"Head branch was modified. Review and try the merge again."}`,
68+
))
69+
}),
70+
}))
71+
72+
deps := BaseDeps{Client: client}
73+
handler := serverTool.Handler(deps)
74+
75+
request := createMCPRequest(map[string]any{
76+
"owner": "owner",
77+
"repo": "repo",
78+
"pullNumber": float64(42),
79+
"merge_method": "merge",
80+
"expectedHeadSha": pinnedSHA,
81+
})
82+
83+
result, err := handler(
84+
ContextWithDeps(context.Background(), deps),
85+
&request,
86+
)
87+
88+
require.NoError(t, err)
89+
require.True(t, result.IsError)
90+
assert.Contains(t, getErrorResult(t, result).Text, "Head branch was modified")
91+
assert.Equal(t, 1, calls, "merge must not retry after a head-SHA mismatch")
92+
})
93+
}

‎pkg/github/pullrequests_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -760,6 +760,7 @@ func Test_MergePullRequest(t *testing.T) {
760760
assert.Contains(t, schema.Properties, "commit_title")
761761
assert.Contains(t, schema.Properties, "commit_message")
762762
assert.Contains(t, schema.Properties, "merge_method")
763+
assert.Contains(t, schema.Properties, "expectedHeadSha")
763764
assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "pullNumber"})
764765

765766
// Setup mock merge result for success case
@@ -778,7 +779,7 @@ func Test_MergePullRequest(t *testing.T) {
778779
expectedErrMsg string
779780
}{
780781
{
781-
name: "successful merge",
782+
name: "successful merge without expected head SHA",
782783
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
783784
PutReposPullsMergeByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{
784785
"commit_title": "Merge PR #42",

0 commit comments

Comments
 (0)