Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1182,6 +1182,7 @@ The following sets of tools are available:
- **OAuth Challenge Scopes**: `repo`
- `commit_message`: Extra detail for merge commit (string, optional)
- `commit_title`: Title for merge commit (string, optional)
- `expectedHeadSha`: The expected SHA of the pull request's HEAD ref (string, optional)
- `merge_method`: Merge method (string, optional)
- `owner`: Repository owner (string, required)
- `pullNumber`: Pull request number (number, required)
Expand Down
6 changes: 5 additions & 1 deletion pkg/github/__toolsnaps__/merge_pull_request.snap
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@
"description": "Title for merge commit",
"type": "string"
},
"expectedHeadSha": {
"description": "The expected SHA of the pull request's HEAD ref",
"type": "string"
},
"merge_method": {
"description": "Merge method",
"enum": [
Expand Down Expand Up @@ -57,4 +61,4 @@
"type": "object"
},
"name": "merge_pull_request"
}
}
9 changes: 9 additions & 0 deletions pkg/github/pullrequests.go
Original file line number Diff line number Diff line change
Expand Up @@ -1513,6 +1513,10 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool
Description: "Merge method",
Enum: []any{"merge", "squash", "rebase"},
},
"expectedHeadSha": {
Type: "string",
Description: "The expected SHA of the pull request's HEAD ref",
},
},
Required: []string{"owner", "repo", "pullNumber"},
}
Expand Down Expand Up @@ -1555,9 +1559,14 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool
if err != nil {
return utils.NewToolResultError(err.Error()), nil, nil
}
expectedHeadSHA, err := OptionalParam[string](args, "expectedHeadSha")
if err != nil {
return utils.NewToolResultError(err.Error()), nil, nil
}

options := &github.PullRequestOptions{
CommitTitle: commitTitle,
SHA: expectedHeadSHA,
MergeMethod: mergeMethod,
}

Expand Down
97 changes: 97 additions & 0 deletions pkg/github/pullrequests_sha_pinning_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package github

import (
"context"
"net/http"
"testing"

"github.com/github/github-mcp-server/pkg/translations"
"github.com/google/go-github/v89/github"
"github.com/google/jsonschema-go/jsonschema"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func Test_MergePullRequestSHAPinning(t *testing.T) {
serverTool := MergePullRequest(translations.NullTranslationHelper)

schema := serverTool.Tool.InputSchema.(*jsonschema.Schema)
require.Contains(t, schema.Properties, "expectedHeadSha")
assert.Equal(t, "string", schema.Properties["expectedHeadSha"].Type)

const pinnedSHA = "0123456789abcdef0123456789abcdef01234567"

mockMergeResult := &github.PullRequestMergeResult{
SHA: github.Ptr("merge-result-sha"),
Merged: github.Ptr(true),
Message: github.Ptr("Pull Request successfully merged"),
}

t.Run("expected head SHA is sent as REST sha", func(t *testing.T) {
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
PutReposPullsMergeByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{
"merge_method": "merge",
"sha": pinnedSHA,
}).andThen(
mockResponse(t, http.StatusOK, mockMergeResult),
),
}))

deps := BaseDeps{Client: client}
handler := serverTool.Handler(deps)

request := createMCPRequest(map[string]any{
"owner": "owner",
"repo": "repo",
"pullNumber": float64(42),
"merge_method": "merge",
"expectedHeadSha": pinnedSHA,
})

result, err := handler(
ContextWithDeps(context.Background(), deps),
&request,
)

require.NoError(t, err)
require.False(t, result.IsError)
})

t.Run("head mismatch fails closed without retry", func(t *testing.T) {
calls := 0

client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
PutReposPullsMergeByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{
"merge_method": "merge",
"sha": pinnedSHA,
}).andThen(func(w http.ResponseWriter, _ *http.Request) {
calls++
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusConflict)
_, _ = w.Write([]byte(
`{"message":"Head branch was modified. Review and try the merge again."}`,
))
}),
}))

deps := BaseDeps{Client: client}
handler := serverTool.Handler(deps)

request := createMCPRequest(map[string]any{
"owner": "owner",
"repo": "repo",
"pullNumber": float64(42),
"merge_method": "merge",
"expectedHeadSha": pinnedSHA,
})

result, err := handler(
ContextWithDeps(context.Background(), deps),
&request,
)

require.NoError(t, err)
require.True(t, result.IsError)
assert.Equal(t, 1, calls, "merge must not retry after a head-SHA mismatch")
})
}