Skip to content

Add expectedHeadSha pinning to merge_pull_request - #3182

Open
jcosta1970 wants to merge 2 commits into
github:mainfrom
jcosta1970:feat/merge-pull-request-expected-head-sha
Open

Add expectedHeadSha pinning to merge_pull_request#3182
jcosta1970 wants to merge 2 commits into
github:mainfrom
jcosta1970:feat/merge-pull-request-expected-head-sha

Conversation

@jcosta1970

Copy link
Copy Markdown

Summary

Adds optional expectedHeadSha pinning to merge_pull_request.

The value is forwarded to github.PullRequestOptions.SHA, which
go-github serializes as the GitHub REST merge endpoint's optional
sha field.

This closes the TOCTOU gap between reviewing a pull request HEAD and merging
it: if the HEAD changes, GitHub rejects the merge instead of merging the new
commit.

Changes

  • add optional expectedHeadSha to the merge_pull_request schema;
  • pass it to PullRequestOptions.SHA;
  • verify the value reaches the REST request as sha;
  • verify a 409 head mismatch fails closed with no retry;
  • update the merge tool schema snapshot;
  • regenerate project docs when local Go tooling is available.

Backward compatibility

The new field is optional. Existing callers that omit
expectedHeadSha retain the current behavior.

Local validation

  • Go tests: SKIPPED_GO_NOT_INSTALLED
  • generated docs: SKIPPED_GO_NOT_INSTALLED
  • golangci-lint: SKIPPED_GOLANGCI_LINT_NOT_INSTALLED
  • git diff --check: PASS

Fixes #3181

@jcosta1970
jcosta1970 requested a review from a team as a code owner August 29, 2026 21:56

@CAOShurong CAOShurong left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head ef6e104f1373ae12a9529bbc8ed4d8041f643fd1.

The optional input is passed directly to go-github's PullRequestOptions.SHA, which serializes it as the REST merge endpoint's sha guard. The focused tests cover both the successful request body and the important failure path: a 409 head mismatch returns an MCP error after exactly one request, so the handler does not silently retry against a different HEAD. Omitting the field keeps the existing behavior.

Verification on this exact head:

  • go test ./pkg/github -run '^Test_MergePullRequestSHAPinning$' -count=1 — pass
  • go vet ./pkg/github — pass
  • go build ./... — pass
  • git diff --check — pass

The full go test ./pkg/github -count=1 run still reports the Windows toolsnap icon-order diffs; the same failures reproduce on exact base febc3293a4feb70e62399f39a26b082f78b9b176, so they are not introduced by this patch.

I did not find a blocking issue in the SHA-pinning change.

AI assistance disclosure: Codex assisted with the local verification and base comparison; I verified the cited heads, diff, and results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add atomic head SHA pinning to merge_pull_request

3 participants