Skip to content

Commit 7ef207e

Browse files
authored
fix(auto_submit): Pass requestSha to GitHub merge API (#5107)
Defense in depth - prevent autosubmit from trying to land a pushed (but not approved) pr. This is not a real issue since we have branch protection rules, but it's good to have.
1 parent a86ab14 commit 7ef207e

3 files changed

Lines changed: 49 additions & 3 deletions

File tree

auto_submit/lib/service/validation_service.dart

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,20 @@ ${pullRequest.title!.replaceFirst('Revert "Revert', 'Reland')}
8282
if (pullRequest.isMergeQueueEnabled) {
8383
return _enqueuePullRequest(slug, pullRequest);
8484
} else {
85-
return _mergePullRequest(number, commitMessage, slug);
85+
if (pullRequest.head?.sha == null) {
86+
return (
87+
result: false,
88+
message:
89+
'Failed to merge ${slug.fullName}/#${pullRequest.number}: invalid head',
90+
method: SubmitMethod.merge,
91+
);
92+
}
93+
return _mergePullRequest(
94+
number,
95+
commitMessage,
96+
slug,
97+
requestSha: pullRequest.head!.sha!,
98+
);
8699
}
87100
}
88101

@@ -122,8 +135,9 @@ ${pullRequest.title!.replaceFirst('Revert "Revert', 'Reland')}
122135
Future<MergeResult> _mergePullRequest(
123136
int number,
124137
String commitMessage,
125-
github.RepositorySlug slug,
126-
) async {
138+
github.RepositorySlug slug, {
139+
required String requestSha,
140+
}) async {
127141
try {
128142
github.PullRequestMerge? result;
129143

@@ -134,6 +148,7 @@ ${pullRequest.title!.replaceFirst('Revert "Revert', 'Reland')}
134148
slug: slug,
135149
number: number,
136150
mergeMethod: github.MergeMethod.squash,
151+
requestSha: requestSha,
137152
);
138153
}, retryIf: (Exception e) => e is RetryableException);
139154

auto_submit/test/service/pull_request_validation_service_test.dart

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,33 @@ void main() {
288288
);
289289

290290
expect(result.message, contains('Reland "My first PR!"'));
291+
expect(githubService.mergePrShaMap[0], pullRequest.head?.sha);
292+
});
293+
294+
test('Fails for invalid sha', () async {
295+
final pullRequest = generatePullRequest(
296+
prNumber: 1001,
297+
repoName: slug.name,
298+
title: 'Revert "Revert "My first PR!"',
299+
mergeable: true,
300+
);
301+
pullRequest.head!.sha = null;
302+
githubService.pullRequestData = pullRequest;
303+
githubService.mergeRequestMock = PullRequestMerge(
304+
merged: true,
305+
sha: pullRequest.mergeCommitSha,
306+
);
307+
308+
final result = await validationService.submitPullRequest(
309+
config: config,
310+
pullRequest: pullRequest,
311+
);
312+
313+
expect(
314+
result.message,
315+
contains('Failed to merge flutter/cocoon/#1001: invalid head'),
316+
);
317+
expect(githubService.mergePrShaMap[1001], isNull);
291318
});
292319

293320
test(

auto_submit/test/src/service/fake_github_service.dart

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,9 @@ class FakeGithubService implements GithubService {
341341

342342
bool throwExceptionOnMerge = false;
343343

344+
/// Recorded commit shas made during [mergePullRequest].
345+
Map<int, String?> mergePrShaMap = <int, String?>{};
346+
344347
/// If useMergeRequestMockList is true then we will return elements from that
345348
/// list until it is empty.
346349
///
@@ -358,6 +361,7 @@ class FakeGithubService implements GithubService {
358361
throw Exception('Exception occurred during merging of pull request.');
359362
}
360363
verifyPullRequestMergeCallMap[number] = slug;
364+
mergePrShaMap[number] = requestSha;
361365
if (useMergeRequestMockList) {
362366
return pullRequestMergeMockList.removeAt(0);
363367
} else {

0 commit comments

Comments
 (0)