From 2cee365440fcaa37bca56759b5cbbcda57ecfc64 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Wed, 22 Apr 2026 11:32:53 -0700 Subject: [PATCH 1/3] Overhaul CICD label management for presubmits This change overhauls the state management for the `CICD` label to ensure trusted users can run CI seamlessly and external contributors require explicit approval. - PR Opened: Privileged users (non-draft) auto-trigger CI and get the label. Non-privileged or draft PRs go to awaiting state. - Changes Pushed (Synchronize): Privileged users trigger CI if label is present. Non-privileged users have the label removed and go to awaiting state. - Edits: Removed automatic label adding on base branch changes. - Consolidated usage of `_scheduleIfMergeable` to ensure consistent conflict checking and label processing. - Added flowchart documentation in `app_dart/docs/cicd_flowchart.md`. --- app_dart/docs/cicd_flowchart.md | 33 +++++++++ .../github/webhook_subscription.dart | 67 +++++++++++------ .../github/webhook_subscription_test.dart | 73 ++++++++++++++----- 3 files changed, 132 insertions(+), 41 deletions(-) create mode 100644 app_dart/docs/cicd_flowchart.md diff --git a/app_dart/docs/cicd_flowchart.md b/app_dart/docs/cicd_flowchart.md new file mode 100644 index 0000000000..c83d1fc23f --- /dev/null +++ b/app_dart/docs/cicd_flowchart.md @@ -0,0 +1,33 @@ +# CICD Label Flowchart + +This flowchart describes the logic for managing the `CICD` label in Cocoon for running presubmit CI. + +```mermaid +flowchart TD + PR_Opened([Event: PR Opened]) --> Is_Draft{Is Draft?} + + Is_Draft -- No --> Is_Privileged_Open{Is Privileged?} + Is_Draft -- Yes --> Create_Awaiting[Create Awaiting Check Run] + + Is_Privileged_Open -- Yes --> Add_Label[Add CICD Label] + Add_Label --> Start_Pre[Start Presubmits] + + Is_Privileged_Open -- No --> Create_Awaiting + + Create_Awaiting --> State_Awaiting((State: Awaiting)) + Start_Pre --> State_Running((State: Running)) + + State_Awaiting --> Label_Added([Event: CICD Label Added]) + Label_Added --> Resolve_Awaiting[Resolve Awaiting Check Run] + Resolve_Awaiting --> Start_Pre + + State_Running --> Changes_Pushed([Event: Changes Pushed]) + Changes_Pushed --> Is_Privileged_Push{Is Privileged?} + Remove_Label --> Create_Awaiting + + Is_Privileged_Push -- No --> Remove_Label[Remove CICD Label] + + Is_Privileged_Push -- Yes --> Label_Present{Has CICD Label?} + Label_Present -- Yes --> Start_Pre + Label_Present -- No --> Create_Awaiting +``` diff --git a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart index fb94076a76..57ede033bb 100644 --- a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart +++ b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart @@ -173,6 +173,8 @@ final class GithubWebhookSubscription extends SubscriptionHandler { /// retried with exponential backoff within that time period. The GoB mirror /// should be caught up within that time frame via either the internal /// mirroring service or [VacuumGithubCommits]. + /// + /// See docs/cicd_flowchart.md for the flowchart of CICD label management. Future _handlePullRequest(String rawRequest) async { final pullRequestEvent = _getPullRequestEvent(rawRequest); if (pullRequestEvent == null) { @@ -199,16 +201,20 @@ final class GithubWebhookSubscription extends SubscriptionHandler { final result = await _processPullRequestClosed(pullRequestEvent); return result.toResponse(); case 'edited': - if (pullRequestEvent.changes != null && - pullRequestEvent.changes!.base != null) { - await _addCICDForRollersAndMembers(pullRequestEvent); - } - await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!); await _checkForTests(pullRequestEvent); break; case 'opened': - await _addCICDForRollersAndMembers(pullRequestEvent); - await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!); + final isPrivileged = await _isPrivilegedUser(pr, slug); + if (!pr.draft! && isPrivileged) { + final gitHubClient = await config.createGitHubClient(pullRequest: pr); + await gitHubClient.issues.addLabelsToIssue(slug, pr.number!, [ + 'CICD', + ]); + log.debug('Added CICD label to PR ${pr.number}'); + await _scheduleIfMergeable(pullRequestEvent); + } else { + await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!); + } await _checkForTests(pullRequestEvent); await _tryReleaseApproval(pullRequestEvent); break; @@ -242,7 +248,31 @@ final class GithubWebhookSubscription extends SubscriptionHandler { await _respondToPullRequestDequeued(pullRequestEvent); break; case 'synchronize': - await _addCICDForRollersAndMembers(pullRequestEvent); + final isPrivileged = await _isPrivilegedUser(pr, slug); + final hasLabel = + pr.labels?.any((l) => l.name == Config.kCicdLabel) ?? false; + final githubService = await config.createGithubService(slug); + + if (isPrivileged) { + if (hasLabel) { + await _scheduleIfMergeable(pullRequestEvent); + } else { + await scheduler.createAwaitingCicdLabelCheckRun( + slug, + pr.head!.sha!, + ); + } + } else { + if (hasLabel) { + await githubService.removeLabel( + slug, + pr.number!, + Config.kCicdLabel, + ); + log.debug('Removed CICD label from PR ${pr.number}'); + } + await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!); + } break; // Ignore the rest of the events. case 'ready_for_review': @@ -580,27 +610,16 @@ final class GithubWebhookSubscription extends SubscriptionHandler { ); } - Future _addCICDForRollersAndMembers( - PullRequestEvent pullRequestEvent, - ) async { - final pr = pullRequestEvent.pullRequest!; - final slug = pr.base!.repo!.slug(); - final author = pr.user!.login!; - final githubService = await config.createGithubService(slug); - + /// Returns true if the user is a roller or a member of flutter-hackers. + Future _isPrivilegedUser(PullRequest pr, RepositorySlug slug) async { final isRoller = config.rollerAccounts.contains(pr.user!.login); + final githubService = await config.createGithubService(slug); final isFlutterHacker = await githubService.isTeamMember( 'flutter-hackers', - author, + pr.user!.login!, slug.owner, ); - log.debug('isRoller=$isRoller, isFlutterHacker=$isFlutterHacker'); - - if (config.supportedRepos.contains(slug) && (isRoller || isFlutterHacker)) { - final gitHubClient = await config.createGitHubClient(pullRequest: pr); - await gitHubClient.issues.addLabelsToIssue(slug, pr.number!, ['CICD']); - log.debug('Added CICD label to PR $pr.number'); - } + return isRoller || isFlutterHacker; } Future _checkForTests(PullRequestEvent pullRequestEvent) async { diff --git a/app_dart/test/request_handlers/github/webhook_subscription_test.dart b/app_dart/test/request_handlers/github/webhook_subscription_test.dart index 854a6c1c0e..19fbbb1c1f 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -3755,7 +3755,7 @@ void foo() { }); test( - 'opened PR with adds CICD label if author is member of flutter-hackers', + 'opened PR with adds CICD label and schedules tests if author is member of flutter-hackers', () async { tester.message = generateGithubWebhookMessage( action: 'opened', @@ -3766,7 +3766,8 @@ void foo() { verify( issuesService.addLabelsToIssue(Config.flutterSlug, 123, ['CICD']), - ); + ).called(1); + expect(scheduler.triggerPresubmitTargetsCnt, 1); }, ); test( @@ -3861,50 +3862,87 @@ void foo() { ); test( - 'synchronize event with CICD label does not schedules tests', + 'synchronize event by non-privileged user removes CICD label and creates awaiting checkrun', () async { + githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( action: 'synchronize', withCicdLabel: true, ); await tester.post(webhook); + + expect(githubService.removedLabels, [ + (Config.flutterSlug, 123, Config.kCicdLabel), + ]); + verify( + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + 'Awaiting CICD label', + output: anyNamed('output'), + ), + ).called(1); expect(scheduler.triggerPresubmitTargetsCnt, 0); }, ); test( - 'synchronize event adds CICD label if author is member of flutter-hackers', + 'synchronize event by non-privileged user without label creates awaiting checkrun', () async { + githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( action: 'synchronize', - login: 'test-flutter-hacker', + withCicdLabel: false, ); await tester.post(webhook); + verify( - issuesService.addLabelsToIssue(Config.flutterSlug, 123, ['CICD']), - ); + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + 'Awaiting CICD label', + output: anyNamed('output'), + ), + ).called(1); + expect(scheduler.triggerPresubmitTargetsCnt, 0); }, ); + test( - 'synchronize event does not add CICD label if author is not member of flutter-hackers', + 'synchronize event by privileged user with label schedules tests', () async { - tester.message = generateGithubWebhookMessage(action: 'synchronize'); + tester.message = generateGithubWebhookMessage( + action: 'synchronize', + login: 'test-flutter-hacker', + withCicdLabel: true, + ); await tester.post(webhook); + + expect(scheduler.triggerPresubmitTargetsCnt, 1); verifyNever( - issuesService.addLabelsToIssue(Config.flutterSlug, 123, any), + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + 'Awaiting CICD label', + output: anyNamed('output'), + ), ); }, ); test( - 'opened PR without CICD label creates "Awaiting CICD label" checkrun', + 'synchronize event by privileged user without label creates awaiting checkrun', () async { githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( - action: 'opened', + action: 'synchronize', + login: 'test-flutter-hacker', withCicdLabel: false, ); @@ -3919,16 +3957,17 @@ void foo() { output: anyNamed('output'), ), ).called(1); + expect(scheduler.triggerPresubmitTargetsCnt, 0); }, ); test( - 'opened PR with CICD label STILL creates "Awaiting CICD label" checkrun if not exists', + 'opened PR without CICD label creates "Awaiting CICD label" checkrun', () async { githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( action: 'opened', - withCicdLabel: true, + withCicdLabel: false, ); await tester.post(webhook); @@ -3946,12 +3985,12 @@ void foo() { ); test( - 'edited PR without CICD label creates "Awaiting CICD label" checkrun if not exists', + 'opened PR with CICD label STILL creates "Awaiting CICD label" checkrun if not exists', () async { githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( - action: 'edited', - withCicdLabel: false, + action: 'opened', + withCicdLabel: true, ); await tester.post(webhook); From 47b088ad91503652ffcf01b4d73ada95863d3ba0 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Wed, 22 Apr 2026 11:43:01 -0700 Subject: [PATCH 2/3] Remove trailing whitespace. --- app_dart/docs/cicd_flowchart.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app_dart/docs/cicd_flowchart.md b/app_dart/docs/cicd_flowchart.md index c83d1fc23f..bdd1eb6f0f 100644 --- a/app_dart/docs/cicd_flowchart.md +++ b/app_dart/docs/cicd_flowchart.md @@ -5,15 +5,15 @@ This flowchart describes the logic for managing the `CICD` label in Cocoon for r ```mermaid flowchart TD PR_Opened([Event: PR Opened]) --> Is_Draft{Is Draft?} - + Is_Draft -- No --> Is_Privileged_Open{Is Privileged?} Is_Draft -- Yes --> Create_Awaiting[Create Awaiting Check Run] - + Is_Privileged_Open -- Yes --> Add_Label[Add CICD Label] Add_Label --> Start_Pre[Start Presubmits] - + Is_Privileged_Open -- No --> Create_Awaiting - + Create_Awaiting --> State_Awaiting((State: Awaiting)) Start_Pre --> State_Running((State: Running)) From 8888a8b435e807047b6a613adde67264fb62bb3a Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Wed, 22 Apr 2026 14:02:31 -0700 Subject: [PATCH 3/3] Explicitly pass flutter as the org name when querying flutter-hackers. --- .../lib/src/request_handlers/github/webhook_subscription.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart index 57ede033bb..e1e7b3a68d 100644 --- a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart +++ b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart @@ -617,7 +617,7 @@ final class GithubWebhookSubscription extends SubscriptionHandler { final isFlutterHacker = await githubService.isTeamMember( 'flutter-hackers', pr.user!.login!, - slug.owner, + 'flutter', ); return isRoller || isFlutterHacker; }