Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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")
})
}