Add expectedHeadSha pinning to merge_pull_request - #3182
Conversation
CAOShurong
left a comment
There was a problem hiding this comment.
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— passgo vet ./pkg/github— passgo build ./...— passgit 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.
Summary
Adds optional
expectedHeadShapinning tomerge_pull_request.The value is forwarded to
github.PullRequestOptions.SHA, whichgo-githubserializes as the GitHub REST merge endpoint's optionalshafield.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
expectedHeadShato themerge_pull_requestschema;PullRequestOptions.SHA;sha;Backward compatibility
The new field is optional. Existing callers that omit
expectedHeadSharetain the current behavior.Local validation
SKIPPED_GO_NOT_INSTALLEDSKIPPED_GO_NOT_INSTALLEDSKIPPED_GOLANGCI_LINT_NOT_INSTALLEDgit diff --check: PASSFixes #3181