Skip to content

Commit cf2c574

Browse files
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>
1 parent ef6e104 commit cf2c574

3 files changed

Lines changed: 6 additions & 9 deletions

File tree

‎docs/feature-flags.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ runtime behavior (such as output formatting) won't appear here.
357357
### `thread_resolution_reason`
358358

359359
- **pull_request_review_write** - Write operations (create, submit, delete) on pull request reviews
360-
- **Required OAuth Scopes**: `repo`
360+
- **OAuth Challenge Scopes**: `repo`
361361
- `body`: Review comment text (string, optional)
362362
- `commitID`: SHA of commit to review (string, optional)
363363
- `event`: Review action to perform. (string, optional)

‎pkg/github/pullrequests_sha_pinning_test.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,13 @@ import (
77

88
"github.com/github/github-mcp-server/pkg/translations"
99
"github.com/google/go-github/v89/github"
10-
"github.com/google/jsonschema-go/jsonschema"
1110
"github.com/stretchr/testify/assert"
1211
"github.com/stretchr/testify/require"
1312
)
1413

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

18-
schema := serverTool.Tool.InputSchema.(*jsonschema.Schema)
19-
require.Contains(t, schema.Properties, "expectedHeadSha")
20-
assert.Equal(t, "string", schema.Properties["expectedHeadSha"].Type)
21-
2217
const pinnedSHA = "0123456789abcdef0123456789abcdef01234567"
2318

2419
mockMergeResult := &github.PullRequestMergeResult{
@@ -44,7 +39,7 @@ func Test_MergePullRequestSHAPinning(t *testing.T) {
4439
"owner": "owner",
4540
"repo": "repo",
4641
"pullNumber": float64(42),
47-
"merge_method": "merge",
42+
"merge_method": "merge",
4843
"expectedHeadSha": pinnedSHA,
4944
})
5045

@@ -81,7 +76,7 @@ func Test_MergePullRequestSHAPinning(t *testing.T) {
8176
"owner": "owner",
8277
"repo": "repo",
8378
"pullNumber": float64(42),
84-
"merge_method": "merge",
79+
"merge_method": "merge",
8580
"expectedHeadSha": pinnedSHA,
8681
})
8782

@@ -92,6 +87,7 @@ func Test_MergePullRequestSHAPinning(t *testing.T) {
9287

9388
require.NoError(t, err)
9489
require.True(t, result.IsError)
90+
assert.Contains(t, getErrorResult(t, result).Text, "Head branch was modified")
9591
assert.Equal(t, 1, calls, "merge must not retry after a head-SHA mismatch")
9692
})
9793
}

‎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)