diff --git a/app_dart/docs/cicd_flowchart.md b/app_dart/docs/cicd_flowchart.md new file mode 100644 index 000000000..79a54c72c --- /dev/null +++ b/app_dart/docs/cicd_flowchart.md @@ -0,0 +1,62 @@ +# 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 +``` + +## Alternative State Machine Diagram (PlantUML) + +```plantuml +@startuml +state PR { + state Waiting + Waiting : entry / Create Awaiting Check Run + Waiting : exit / resolve(awaiting check) + + state Running + Running: entry/ Start Presubmits + + state if_priv2 <> + Running --> if_priv2: onPushed + if_priv2 --> Running: isPrivileged && labeled(CICD) + if_priv2 --> Waiting : default: remove(CICD) + + state if_draft <> + PR --> if_draft: openned + + if_draft --> Waiting : isDraft + if_draft --> Running: isPrivileged + if_draft --> Waiting : default + + Waiting --> Running: labeled(CICD) +} +@enduml +``` diff --git a/app_dart/lib/cocoon_service.dart b/app_dart/lib/cocoon_service.dart index 9cd66123a..6c6e325f0 100644 --- a/app_dart/lib/cocoon_service.dart +++ b/app_dart/lib/cocoon_service.dart @@ -60,4 +60,5 @@ export 'src/service/flags/dynamic_config.dart'; export 'src/service/gerrit_service.dart'; export 'src/service/github_checks_service.dart'; export 'src/service/luci_build_service.dart'; +export 'src/service/pull_request_manager.dart'; export 'src/service/scheduler.dart'; diff --git a/app_dart/lib/src/model/firestore/pull_request_state.dart b/app_dart/lib/src/model/firestore/pull_request_state.dart new file mode 100644 index 000000000..0e5e2c256 --- /dev/null +++ b/app_dart/lib/src/model/firestore/pull_request_state.dart @@ -0,0 +1,85 @@ +// Copyright 2026 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:convert'; + +import 'package:github/github.dart'; +import 'package:googleapis/firestore/v1.dart'; + +import '../../service/firestore.dart'; +import 'base.dart'; + +/// Represents the state of a Pull Request that we want to persist across events. +final class PullRequestState extends AppDocument { + static const kCollectionId = 'pullRequestStates'; + static const kIsPrivilegedField = 'is_privileged'; + static const kLatestShaField = 'latest_sha'; + static const kScheduledShaField = 'scheduled_sha'; + static const kSlugField = 'slug'; + static const kNumberField = 'number'; + + @override + AppDocumentMetadata get runtimeMetadata => metadata; + + /// Description of the document in Firestore. + static final metadata = AppDocumentMetadata( + collectionId: kCollectionId, + fromDocument: PullRequestState.fromDocument, + ); + + /// Create [PullRequestState] from a Document. + static PullRequestState fromDocument(Document doc) { + return PullRequestState() + ..fields = doc.fields! + ..name = doc.name!; + } + + /// Whether the PR author is a privileged user (roller or flutter-hacker). + bool? get isPrivileged => fields[kIsPrivilegedField]?.booleanValue; + + set isPrivileged(bool? value) { + if (value != null) { + fields[kIsPrivilegedField] = value.toValue(); + } + } + + /// The latest SHA that we have processed for this PR. + String? get latestSha => fields[kLatestShaField]?.stringValue; + + set latestSha(String? value) { + if (value != null) { + fields[kLatestShaField] = value.toValue(); + } + } + + /// The SHA for which we have scheduled presubmits. + String? get scheduledSha => fields[kScheduledShaField]?.stringValue; + + set scheduledSha(String? value) { + if (value != null) { + fields[kScheduledShaField] = value.toValue(); + } + } + + /// The repository slug associated with the pull request. + RepositorySlug get slug => RepositorySlug.fromJson( + json.decode(fields[kSlugField]!.stringValue!) as Map, + ); + + set slug(RepositorySlug value) { + fields[kSlugField] = json.encode(value.toJson()).toValue(); + } + + /// The PR number. + int get number => int.parse(fields[kNumberField]!.integerValue!); + + set number(int value) { + fields[kNumberField] = value.toValue(); + } + + /// Generates the document ID for a PR. + static String getDocumentId(RepositorySlug slug, int number) { + return '${slug.owner}\u001F${slug.name}\u001F$number'; + } +} 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 2ab49a5d4..5509741df 100644 --- a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart +++ b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart @@ -9,7 +9,6 @@ import 'package:cocoon_server/logging.dart'; import 'package:github/github.dart'; import 'package:github/github.dart' as github; import 'package:github/hooks.dart'; -import 'package:googleapis/firestore/v1.dart' as g; import 'package:meta/meta.dart'; import '../../../cocoon_service.dart'; @@ -20,36 +19,8 @@ import '../../model/github/workflow_job.dart' as workflow_job; import '../../request_handling/exceptions.dart'; import '../../request_handling/subscription_handler.dart'; import '../../service/commit_service.dart'; -import '../../service/github_service.dart'; import '../../service/scheduler/process_check_run_result.dart'; -// Filenames which are not actually tests. -const List kNotActuallyATest = [ - 'packages/flutter/lib/src/gestures/hit_test.dart', -]; - -/// List of repos that require check for tests. -Set kNeedsTests = { - Config.flutterSlug, - Config.packagesSlug, -}; - -// Extentions for files that use // for single line comments. -// See [_allChangesAreCodeComments] for more. -@visibleForTesting -const Set knownCommentCodeExtensions = { - 'cc', - 'cpp', - 'dart', - 'gradle', - 'groovy', - 'java', - 'kt', - 'm', - 'mm', - 'swift', -}; - /// Subscription for processing GitHub webhooks. /// /// The PubSub subscription is set up here: @@ -65,7 +36,7 @@ final class GithubWebhookSubscription extends SubscriptionHandler { static const _estimatedGitOnBorgMaximumSyncDuration = Duration(minutes: 15); /// Creates a subscription for processing GitHub webhooks. - const GithubWebhookSubscription({ + GithubWebhookSubscription({ required super.cache, required super.config, required this.scheduler, @@ -190,59 +161,54 @@ final class GithubWebhookSubscription extends SubscriptionHandler { throw const InternalServerError('Unsupported repository'); } + final context = PullRequestEventContext( + cache: cache, + firestore: firestore, + config: config, + scheduler: scheduler, + gerritService: gerritService, + pullRequestLabelProcessorProvider: pullRequestLabelProcessorProvider, + ); + // See the API reference: // https://developer.github.com/v3/activity/events/types/#pullrequestevent // which unfortunately is a bit light on explanations. log.info('$crumb: processing $eventAction for ${pr.htmlUrl}'); switch (eventAction) { case 'closed': - final result = await _processPullRequestClosed(pullRequestEvent); - return result.toResponse(); + return await PullRequestManager.handleClosed( + pullRequestEvent, + context, + _processPullRequestClosed, + ); case 'edited': - if (pullRequestEvent.changes != null && - pullRequestEvent.changes!.base != null) { - await _addCICDForRollersAndMembers(pullRequestEvent); - } - await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!); - await _checkForTests(pullRequestEvent); + await PullRequestManager.handleEdited(pullRequestEvent, context); break; case 'opened': - await _addCICDForRollersAndMembers(pullRequestEvent); - await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!); - await _checkForTests(pullRequestEvent); - await _tryReleaseApproval(pullRequestEvent); + await PullRequestManager.handleOpened(pullRequestEvent, context); break; case 'reopened': - // TODO(matanlurey): Handle more elegantly than just failing. - // https://github.com/flutter/flutter/issues/162656 - await _warnThatANewCommitIsNeeded(pullRequestEvent); + await PullRequestManager.handleReopened(pullRequestEvent, context); + break; case 'labeled': log.info( '$crumb: PR labels = [${pr.labels?.map((label) => '"${label.name}"').join(', ')}]', ); - final labelEvent = _getLabeledEvent(rawRequest); - if (labelEvent?.label case final label?) { - if (Config.kCicdLabelIds.contains(label.id) && - label.name == Config.kCicdLabel) { - log.info('new CICD label added - scheduling tests'); - await scheduler.resolveAwaitingCicdLabelCheckRun( - slug, - pr.head!.sha!, - ); - await _scheduleIfMergeable(pullRequestEvent); - } - } - - await _processLabels(pr); - await _updatePullRequest(pr); - + final labelName = labelEvent?.label.name; + final labelId = labelEvent?.label.id; + await PullRequestManager.handleLabeled( + pullRequestEvent, + context, + labelName, + labelId, + ); break; case 'dequeued': await _respondToPullRequestDequeued(pullRequestEvent); break; case 'synchronize': - await _addCICDForRollersAndMembers(pullRequestEvent); + await PullRequestManager.handleSynchronize(pullRequestEvent, context); break; // Ignore the rest of the events. case 'ready_for_review': @@ -258,19 +224,6 @@ final class GithubWebhookSubscription extends SubscriptionHandler { return Response.emptyOk; } - Future _processLabels(PullRequest pullRequest) async { - final slug = pullRequest.base!.repo!.slug(); - final githubService = await config.createGithubService(slug); - - final labelProcessor = pullRequestLabelProcessorProvider( - config: config, - githubService: githubService, - pullRequest: pullRequest, - ); - - return labelProcessor.processLabels(); - } - /// Handles a GitHub webhook with the event type "merge_group". /// /// A merge group contains commits from multiple pull requests. Each pull @@ -413,109 +366,6 @@ final class GithubWebhookSubscription extends SubscriptionHandler { } } - /// This method assumes that jobs should be cancelled if they are already - /// runnning. - Future _scheduleIfMergeable(PullRequestEvent pullRequestEvent) async { - final pr = pullRequestEvent.pullRequest!; - final slug = pullRequestEvent.repository!.slug(); - - log.info( - 'Scheduling tasks if mergeable(${pr.mergeable}): owner=${slug.owner} repo=${slug.name} and pr=${pr.number}', - ); - - // The mergeable flag may be null. False indicates there's a merge conflict, - // null indicates unknown. Err on the side of allowing the job to run. - if (pr.mergeable == false) { - final slug = pullRequestEvent.repository!.slug(); - final gitHubClient = await config.createGitHubClient(pullRequest: pr); - final body = config.mergeConflictPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - return; - } - - try { - await scheduler.triggerPresubmitTargets(pullRequest: pr); - - // When presubmit targets are scheduled the PR acquires a new Merge Queue - // Guard. This can happen when the PR is just created, a new commit is - // pushed, reopened, etc. In all cases the guard may need to be unlocked if, - // for example, the "emergency" label is present. - await _processLabels(pr); - } on g.DetailedApiRequestError catch (e) { - if (e.status != HttpStatus.conflict) { - rethrow; - } - await _warnThatANewCommitIsNeeded(pullRequestEvent); - } - } - - /// Update the PR stored in [PrCheckRuns] so that subsequent checks are fresh. - Future _updatePullRequest(PullRequest pr) async { - final sha = pr.head!.sha!; - final didUpdate = await PrCheckRuns.updatePullRequestForSha( - firestore, - sha, - pr, - ); - if (!didUpdate) { - log.debug('No PR found for SHA: $sha, did not update'); - } else { - log.debug('Updated PR for SHA: $sha'); - } - } - - /// Warns that [pr] is out of date, and a new commit is needed. - /// - /// See . - Future _warnThatANewCommitIsNeeded( - PullRequestEvent pullRequestEvent, - ) async { - final pr = pullRequestEvent.pullRequest!; - final slug = pullRequestEvent.repository!.slug(); - final sha = pr.head!.sha!; - - final gitHubClient = config.createGitHubClientWithToken( - await config.githubOAuthToken, - ); - await gitHubClient.issues.createComment( - slug, - pr.number!, - Config.newCommitIsNeeded(sha: sha), - ); - } - - /// Release tooling generates cherrypick pull requests that should be granted an approval. - Future _tryReleaseApproval(PullRequestEvent pullRequestEvent) async { - final pr = pullRequestEvent.pullRequest!; - final slug = pullRequestEvent.repository!.slug(); - - final defaultBranch = Config.defaultBranch(slug); - final branch = pr.base?.ref; - if (branch == null || branch.contains(defaultBranch)) { - // This isn't a release branch PR - return; - } - - final releaseAccounts = await config.releaseAccounts; - if (releaseAccounts.contains(pr.user?.login) == false) { - // The author isn't in the list of release accounts, do nothing - return; - } - - final gitHubClient = config.createGitHubClientWithToken( - await config.githubOAuthToken, - ); - final review = CreatePullRequestReview( - slug.owner, - slug.name, - pr.number!, - 'APPROVE', - ); - await gitHubClient.pullRequests.createReview(slug, review); - } - @useResult Future _processPullRequestClosed( PullRequestEvent pullRequestEvent, @@ -580,418 +430,6 @@ 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); - - final isRoller = config.rollerAccounts.contains(pr.user!.login); - final isFlutterHacker = await githubService.isTeamMember( - 'flutter-hackers', - author, - 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'); - } - } - - Future _checkForTests(PullRequestEvent pullRequestEvent) async { - final pr = pullRequestEvent.pullRequest!; - // We do not need to add test labels if this is an auto roller author. - if (config.rollerAccounts.contains(pr.user!.login)) { - return; - } - final eventAction = pullRequestEvent.action; - final slug = pr.base!.repo!.slug(); - final isTipOfTree = pr.base!.ref == Config.defaultBranch(slug); - final gitHubClient = await config.createGitHubClient(pullRequest: pr); - await _validateRefs(gitHubClient, pr); - if (kNeedsTests.contains(slug) && isTipOfTree) { - log.info( - 'Applying framework repo labels for: owner=${slug.owner} repo=${slug.name} and pr=${pr.number}', - ); - switch (slug.name) { - case 'flutter': - final isFusion = slug == Config.flutterSlug; - final files = await gitHubClient.pullRequests - .listFiles(slug, pr.number!) - .toList(); - await _applyFrameworkRepoLabels( - gitHubClient, - eventAction, - pr, - files, - slug, - ); - if (isFusion) { - await _applyEngineRepoLabels( - gitHubClient, - eventAction, - pr, - files, - slug, - ); - } - case 'packages': - return _applyPackageTestChecks(gitHubClient, eventAction, pr); - } - } - } - - Future _applyFrameworkRepoLabels( - GitHub gitHubClient, - String? eventAction, - PullRequest pr, - List files, - RepositorySlug slug, - ) async { - if (pr.user!.login == 'engine-flutter-autoroll') { - return; - } - - final labels = {}; - var hasTests = false; - var needsTests = false; - - var frameworkFiles = 0; - - for (var file in files) { - final filename = file.filename!; - - if (!_isFusionEnginePath(filename)) { - frameworkFiles++; - } - - if (_fileContainsAddedCode(file) && - !_isTestExempt(filename) && - !filename.startsWith('dev/bots/') && - !filename.endsWith('.gitignore')) { - needsTests = !_allChangesAreCodeComments(file); - } - - // Check to see if tests were submitted with this PR. - if (_isAFrameworkTest(filename)) { - hasTests = true; - } - } - - if (frameworkFiles == 0) { - // a fusion / engine only change. - return; - } - - if (pr.user!.login == 'fluttergithubbot') { - needsTests = false; - labels.addAll(['c: tech-debt', 'c: flake']); - } - - if (labels.isNotEmpty) { - await gitHubClient.issues.addLabelsToIssue( - slug, - pr.number!, - labels.toList(), - ); - } - - if (!hasTests && - needsTests && - !pr.draft! && - !_isPrUpdatingReleaseBranch(pr)) { - final body = config.missingTestsPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - } - } - - bool _isAFrameworkTest(String filename) { - if (kNotActuallyATest.any(filename.endsWith)) { - return false; - } - // Check for Objective-C tests which end in either "Tests.m" or "Test.m" - // in the "dev" directory. - final objectiveCTestRegex = RegExp(r'.*dev\/.*Test[s]?\.m$'); - return filename.endsWith('_test.dart') || - filename.endsWith('.expect') || - filename.contains('test_fixes') || - // Include updates to test utilities or test data - filename.contains('packages/flutter_tools/test/') || - // Kotlin source tests, used in the Flutter Gradle Plugin. - filename.startsWith('packages/flutter_tools/gradle/src/test/') || - filename.startsWith('dev/bots/analyze.dart') || - filename.startsWith('dev/bots/test.dart') || - filename.startsWith('dev/devicelab/bin/tasks') || - filename.startsWith('dev/devicelab/lib/tasks') || - filename.startsWith('dev/benchmarks') || - objectiveCTestRegex.hasMatch(filename); - } - - /// Returns true if changes to [filename] are exempt from the testing - /// requirement, across repositories. - bool _isTestExempt(String filename, {String engineBasePath = ''}) { - final isBuildPythonScript = - filename.startsWith('${engineBasePath}sky/tools') && - filename.endsWith('.py'); - return filename.contains('.ci.yaml') || - filename.endsWith('analysis_options.yaml') || - filename.endsWith('AUTHORS') || - filename.endsWith('CODEOWNERS') || - filename.endsWith('TESTOWNERS') || - filename.endsWith('pubspec.yaml') || - filename.endsWith('pubspec.yaml.tmpl') || - // Exempt categories. - filename.contains('.gemini/') || - filename.contains('.github/') || - filename.endsWith('.md') || - // Exempt paths. - filename.startsWith('dev/customer_testing/tests.version') || - filename.startsWith('dev/devicelab/lib/versions/gallery.dart') || - filename.startsWith('dev/integration_tests/') || - filename.startsWith('docs/') || - filename.startsWith('${engineBasePath}docs/') || - filename.endsWith('test/flutter_test_config.dart') || - // ↓↓↓ Begin engine specific paths ↓↓↓ - filename == 'DEPS' || // note: in monorepo; DEPS is still at the root. - isBuildPythonScript || - filename.endsWith('.gni') || - filename.endsWith('.gn') || - filename.startsWith('${engineBasePath}impeller/fixtures/') || - filename.startsWith('${engineBasePath}impeller/golden_tests/') || - filename.startsWith('${engineBasePath}impeller/playground/') || - filename.startsWith( - '${engineBasePath}shell/platform/embedder/tests/', - ) || - filename.startsWith( - '${engineBasePath}shell/platform/embedder/fixtures/', - ) || - filename.startsWith('${engineBasePath}testing/') || - filename.startsWith('${engineBasePath}tools/clangd_check/'); - } - - bool _isFusionEnginePath(String path) => - path.startsWith('engine/') || path == 'DEPS'; - - Future _applyEngineRepoLabels( - GitHub gitHubClient, - String? eventAction, - PullRequest pr, - List files, - RepositorySlug slug, - ) async { - // Do not apply the test labels for the autoroller accounts. - if (pr.user!.login == 'skia-flutter-autoroll') { - return; - } - - var hasTests = false; - var needsTests = false; - - // If engine labels are being applied to the flutterSlug - we're in a fusion repo. - final isFusion = slug == Config.flutterSlug; - final engineBasePath = isFusion ? 'engine/src/flutter/' : ''; - - var engineFiles = 0; - - for (var file in files) { - final path = file.filename!; - if (isFusion && _isFusionEnginePath(path)) { - engineFiles++; - } - - if (_fileContainsAddedCode(file) && - !_isTestExempt(path, engineBasePath: engineBasePath) && - // License goldens are auto-generated. - !path.startsWith('${engineBasePath}ci/licenses_golden/') && - // Build configuration files tell CI what to run. - !path.startsWith('${engineBasePath}ci/builders/')) { - needsTests = !_allChangesAreCodeComments(file); - } - - if (_isAnEngineTest(path)) { - hasTests = true; - } - } - - if (isFusion && engineFiles == 0) { - // framework only change - return; - } - - if (!hasTests && - needsTests && - !pr.draft! && - !_isPrUpdatingReleaseBranch(pr)) { - final body = config.missingTestsPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - } - } - - bool _isAnEngineTest(String filename) { - final engineTestRegExp = RegExp( - r'(tests?|benchmarks?)\.(dart|java|mm|m|cc|sh|py|swift)$', - ); - return filename.contains('IosBenchmarks') || - filename.contains('IosUnitTests') || - filename.contains('scenario_app') || - engineTestRegExp.hasMatch(filename.toLowerCase()); - } - - bool _fileContainsAddedCode(PullRequestFile file) { - // When null, do not assume 0 lines have been added. - final linesAdded = file.additionsCount ?? 1; - final linesDeleted = file.deletionsCount ?? 0; - final linesTotal = file.changesCount ?? linesDeleted + linesAdded; - return linesAdded > 0 || linesDeleted != linesTotal; - } - - // Runs automated test checks for both flutter/packages. - Future _applyPackageTestChecks( - GitHub gitHubClient, - String? eventAction, - PullRequest pr, - ) async { - final slug = pr.base!.repo!.slug(); - final files = gitHubClient.pullRequests.listFiles(slug, pr.number!); - var hasTests = false; - var needsTests = false; - - await for (PullRequestFile file in files) { - final filename = file.filename!; - - if (_fileContainsAddedCode(file) && - !_isTestExempt(filename) && - !filename.contains('.ci/') && - // Custom package-specific test runners. These do not count as tests - // for the purposes of testing a change that otherwise needs tests, - // but since they are the driver for tests they don't need test - // coverage. - !filename.endsWith('tool/run_tests.dart') && - !filename.endsWith('run_tests.sh')) { - needsTests = !_allChangesAreCodeComments(file); - } - // See https://github.com/flutter/flutter/blob/master/docs/ecosystem/testing/Plugin-Tests.md for discussion - // of various plugin test types and locations. - if (filename.endsWith('_test.dart') || - // Native iOS/macOS tests. - filename.contains('RunnerTests/') || - filename.contains('RunnerUITests/') || - filename.contains('darwin/Tests/') || - // Native Android tests. - filename.contains('android/src/test/') || - filename.contains('androidTest/') || - // Native Linux tests. - filename.endsWith('_test.cc') || - // Native Windows tests. - filename.endsWith('_test.cpp') || - // Pigeon native tests. - filename.contains('/platform_tests/') || - // Test files in package-specific test folders. - filename.contains('go_router/test_fixes/') || - filename.contains('go_router_builder/test_inputs/')) { - hasTests = true; - } - } - - if (!hasTests && - needsTests && - !pr.draft! && - !_isPrUpdatingReleaseBranch(pr)) { - final body = config.missingTestsPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - } - } - - /// Validate the base and head refs of the PR. - Future _validateRefs(GitHub gitHubClient, PullRequest pr) async { - final slug = pr.base!.repo!.slug(); - String body; - const releaseChannels = ['stable', 'beta', 'dev']; - // Close PRs that use a release branch as a source. - if (releaseChannels.contains(pr.head!.ref)) { - body = config.wrongHeadBranchPullRequestMessage(pr.head!.ref!); - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.pullRequests.edit(slug, pr.number!, state: 'closed'); - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - return; - } - final defaultBranchName = Config.defaultBranch(pr.base!.repo!.slug()); - final baseName = pr.base!.ref!; - if (baseName == defaultBranchName) { - return; - } - if (_isPrUpdatingReleaseBranch(pr)) { - body = config.releaseBranchPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - return; - } - - // For repos migrated to main, close PRs opened against master. - final isMaster = pr.base?.ref == 'master'; - final isMigrated = defaultBranchName == 'main'; - // PRs should never be open to "beta" or "stable." - final isReleaseChannelBranch = releaseChannels.contains(pr.base?.ref); - if ((isMaster && isMigrated) || isReleaseChannelBranch) { - body = _getWrongBaseComment( - base: baseName, - defaultBranch: defaultBranchName, - ); - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.pullRequests.edit( - slug, - pr.number!, - base: Config.defaultBranch(slug), - ); - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - } - } - - static bool _isPrUpdatingReleaseBranch(PullRequest pr) { - return isReleaseCandidateBranch(branchName: pr.base!.ref!); - } - - Future _alreadyCommented( - GitHub gitHubClient, - PullRequest pr, - String message, - ) async { - final comments = gitHubClient.issues.listCommentsByIssue( - pr.base!.repo!.slug(), - pr.number!, - ); - await for (IssueComment comment in comments) { - if (comment.body != null && comment.body!.contains(message)) { - return true; - } - } - return false; - } - - String _getWrongBaseComment({ - required String base, - required String defaultBranch, - }) { - final messageTemplate = config.wrongBaseBranchPullRequestMessage; - return messageTemplate - .replaceAll('{{target_branch}}', base) - .replaceAll('{{default_branch}}', defaultBranch); - } - PullRequestEvent? _getPullRequestEvent(String request) { try { return PullRequestEvent.fromJson( @@ -1013,163 +451,4 @@ final class GithubWebhookSubscription extends SubscriptionHandler { return null; } } - - /// Returns true if the changes to [file] are all code comments. - /// - /// If that cannot be determined with confidence, returns false. False - /// negatives (e.g., for /* */-style multi-line comments) should be expected. - bool _allChangesAreCodeComments(PullRequestFile file) { - final linesAdded = file.additionsCount; - final linesDeleted = file.deletionsCount; - final patch = file.patch; - // If information is missing, err or the side of assuming it's a non-comment - // change. - if (linesAdded == null || linesDeleted == null || patch == null) { - return false; - } - - final filename = file.filename!; - final extension = filename.contains('.') - ? filename.split('.').last.toLowerCase() - : null; - if (extension == null || !knownCommentCodeExtensions.contains(extension)) { - return false; - } - - // Only handles single-line comments; identifying multi-line comments - // would require the full file and non-trivial parsing. Also doesn't handle - // end-of-line comments (e.g., "int x = 0; // Info about x"). - final commentRegex = RegExp(r'^[+-]\s*//'); - final onlyWhitespaceRegex = RegExp(r'^[+-]\s*$'); - for (var line in patch.split('\n')) { - if (!line.startsWith('+') && !line.startsWith('-')) { - continue; - } - - if (onlyWhitespaceRegex.hasMatch(line)) { - // whitespace only changes don't require tests - continue; - } - - if (!commentRegex.hasMatch(line)) { - return false; - } - } - return true; - } -} - -typedef PullRequestLabelProcessorProvider = - PullRequestLabelProcessor Function({ - required Config config, - required GithubService githubService, - required PullRequest pullRequest, - }); - -class PullRequestLabelProcessor { - PullRequestLabelProcessor({ - required this.config, - required this.githubService, - required this.pullRequest, - }) : slug = pullRequest.base!.repo!.slug(), - prNumber = pullRequest.number!; - - static const kEmergencyLabelEducation = ''' -Detected the `emergency` label. - -If you add the `autosubmit` label, the bot will wait until all presubmits pass but ignore the tree status, allowing fixes for tree breakages while still validating that they don't break any existing presubmits. - -The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". -'''; - - final Config config; - final GithubService githubService; - final PullRequest pullRequest; - final RepositorySlug slug; - final int prNumber; - late final String logCrumb = - '$PullRequestLabelProcessor($slug/pull/$prNumber)'; - - void logInfo(Object? message) { - log.info('$logCrumb: $message'); - } - - void logSevere(Object? message, {Object? error, StackTrace? stackTrace}) { - log.error('$logCrumb: $message', error, stackTrace); - } - - Future processLabels() async { - final hasEmergencyLabel = - pullRequest.labels?.any( - (label) => label.name == Config.kEmergencyLabel, - ) ?? - false; - if (hasEmergencyLabel) { - // The merge guard and Flutter Presubmits check can be unlocked without approval checks because: - // - // * For manual merges the GitHub repo settings already require minimum - // approvals before the PR can be submitted. - // * For `autosubmit` label Cocoon has the [Approval] validation that - // checks approvals before attempting to merge the PR. - await _unlockCheckrunsForEmergency(); - } else { - logInfo('no emergency label; moving on.'); - } - } - - Future _unlockCheckrunsForEmergency() async { - await _unlockCheckrun(Config.kMergeQueueLockName); - await _unlockCheckrun(Config.kFlutterPresubmitsName); - - // Let the developer know what is happening with the MQ when this label is found the first time. - try { - if (!await githubService.commentExists( - slug, - prNumber, - PullRequestLabelProcessor.kEmergencyLabelEducation, - )) { - await githubService.createComment( - slug, - issueNumber: prNumber, - body: PullRequestLabelProcessor.kEmergencyLabelEducation, - ); - } - } catch (e, s) { - logSevere( - 'failed to leave educational comment for emergency label.', - error: e, - stackTrace: s, - ); - } - } - - Future _unlockCheckrun(String checkName) async { - logInfo('attempting to unlock the $checkName for emergency'); - - final guard = (await githubService.getCheckRunsFiltered( - slug: slug, - ref: pullRequest.head!.sha!, - checkName: checkName, - )).singleOrNull; - - if (guard == null) { - logSevere( - 'failed to process the emergency label. "$checkName" check run is missing.', - ); - return; - } - - await githubService.updateCheckRun( - slug: slug, - checkRun: guard, - status: CheckRunStatus.completed, - conclusion: CheckRunConclusion.success, - output: CheckRunOutput( - title: checkName, - summary: 'Emergency label applied.', - ), - ); - - logInfo('unlocked "$checkName", allowing it to land as an emergency.'); - } } diff --git a/app_dart/lib/src/service/pull_request_manager.dart b/app_dart/lib/src/service/pull_request_manager.dart new file mode 100644 index 000000000..9d4b01955 --- /dev/null +++ b/app_dart/lib/src/service/pull_request_manager.dart @@ -0,0 +1,1132 @@ +// Copyright 2026 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; +import 'dart:collection'; +import 'dart:io'; +import 'dart:typed_data'; + +import 'package:cocoon_common/is_release_branch.dart'; +import 'package:cocoon_server/logging.dart'; +import 'package:github/github.dart'; +import 'package:github/hooks.dart'; +import 'package:googleapis/firestore/v1.dart' hide Value; + +import '../model/firestore/pr_check_runs.dart'; +import '../model/firestore/pull_request_state.dart'; +import '../request_handling/exceptions.dart'; +import '../request_handling/response.dart'; +import 'cache_service.dart'; +import 'config.dart'; +import 'firestore.dart'; +import 'gerrit_service.dart'; +import 'github_service.dart'; +import 'scheduler.dart'; +import 'scheduler/process_check_run_result.dart'; + +// Filenames which are not actually tests. +const List kNotActuallyATest = [ + 'packages/flutter/lib/src/gestures/hit_test.dart', +]; + +/// List of repos that require check for tests. +Set kNeedsTests = { + Config.flutterSlug, + Config.packagesSlug, +}; + +// Extentions for files that use // for single line comments. +const Set knownCommentCodeExtensions = { + 'cc', + 'cpp', + 'dart', + 'gradle', + 'groovy', + 'java', + 'kt', + 'm', + 'mm', + 'swift', +}; + +/// Manages the state and event queue for a single Pull Request. +class PullRequestManager { + final RepositorySlug slug; + final int prNumber; + final FirestoreService firestore; + final Config config; + final Scheduler scheduler; + final GerritService gerritService; + final PullRequestLabelProcessorProvider pullRequestLabelProcessorProvider; + + final bool _isPrivileged; + String _latestSha; + String? _scheduledSha; + bool _isDirty = false; + + PullRequestManager._({ + required this.slug, + required this.prNumber, + required this.firestore, + required this.config, + required this.scheduler, + required this.gerritService, + required this.pullRequestLabelProcessorProvider, + required bool isPrivileged, + required String latestSha, + String? scheduledSha, + }) : _isPrivileged = isPrivileged, + _latestSha = latestSha, + _scheduledSha = scheduledSha; + + /// Runs an action with a distributed lock on the pull request. + static Future _runWithLock({ + required PullRequestEvent event, + required CacheService cache, + required Future Function() action, + }) async { + final pr = event.pullRequest!; + final slug = event.repository!.slug(); + final prNumber = pr.number!; + final lockKey = 'pr_lock_${slug.owner}_${slug.name}_$prNumber'; + final lockValue = Uint8List.fromList('l'.codeUnits); + + // Attempt to acquire lock + final existingLock = await cache.getOrCreate( + 'pr_locks', + lockKey, + createFn: null, + ); + if (existingLock != null) { + throw const ServiceUnavailable('PR is locked by another instance'); + } + await cache.set( + 'pr_locks', + lockKey, + lockValue, + ttl: const Duration(minutes: 5), + ); + + try { + return await action(); + } finally { + await cache.purge('pr_locks', lockKey); + } + } + + static Future handleOpened( + PullRequestEvent event, + PullRequestEventContext context, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, + ); + await manager._handleOpened(event); + await manager.persist(); + }, + ); + } + + static Future handleSynchronize( + PullRequestEvent event, + PullRequestEventContext context, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, + ); + await manager._handleSynchronize(event); + await manager.persist(); + }, + ); + } + + static Future handleEdited( + PullRequestEvent event, + PullRequestEventContext context, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, + ); + await manager._handleEdited(event); + await manager.persist(); + }, + ); + } + + static Future handleReopened( + PullRequestEvent event, + PullRequestEventContext context, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, + ); + await manager._handleReopened(event); + await manager.persist(); + }, + ); + } + + static Future handleLabeled( + PullRequestEvent event, + PullRequestEventContext context, + String? labelName, + int? labelId, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, + ); + await manager._handleLabeled(event, labelName, labelId); + await manager.persist(); + }, + ); + } + + static Future handleClosed( + PullRequestEvent event, + PullRequestEventContext context, + Future Function(PullRequestEvent) processClosedFn, + ) async { + return await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final result = await processClosedFn(event); + return result.toResponse(); + }, + ); + } + + /// Creates and hydrates a [PullRequestManager] instance. + static Future _create({ + required RepositorySlug slug, + required int prNumber, + required FirestoreService firestore, + required Config config, + required Scheduler scheduler, + required GerritService gerritService, + required PullRequestLabelProcessorProvider + pullRequestLabelProcessorProvider, + required PullRequestEvent event, + }) async { + final documentId = PullRequestState.getDocumentId(slug, prNumber); + final name = + '$kDocumentParent/${PullRequestState.kCollectionId}/$documentId'; + + bool isPrivileged; + String latestSha; + String? scheduledSha; + + try { + final doc = await firestore.getDocument(name); + final state = PullRequestState.fromDocument(doc); + isPrivileged = state.isPrivileged ?? false; + latestSha = state.latestSha ?? event.pullRequest!.head!.sha!; + scheduledSha = state.scheduledSha; + log.info( + 'Hydrated PullRequestManager for $slug/$prNumber: isPrivileged=$isPrivileged, latestSha=$latestSha, scheduledSha=$scheduledSha', + ); + } on DetailedApiRequestError catch (e) { + if (e.status != HttpStatus.notFound) { + rethrow; + } + + // Document not found, initialize as new + final pr = event.pullRequest!; + final author = pr.user!.login!; + final githubService = await config.createGithubService(slug); + final isRoller = config.rollerAccounts.contains(author); + final isFlutterHacker = await githubService.isTeamMember( + 'flutter-hackers', + author, + slug.owner, + ); + isPrivileged = isRoller || isFlutterHacker; + latestSha = pr.head!.sha!; + + // Persist initial state using the specific document ID! + final state = PullRequestState() + ..slug = slug + ..number = prNumber + ..isPrivileged = isPrivileged + ..latestSha = latestSha; + + final document = Document(fields: state.fields); + await firestore.createDocument( + document, + collectionId: PullRequestState.kCollectionId, + documentId: documentId, + ); + log.info('Created initial PullRequestState for $slug/$prNumber'); + } + + return PullRequestManager._( + slug: slug, + prNumber: prNumber, + firestore: firestore, + config: config, + scheduler: scheduler, + gerritService: gerritService, + pullRequestLabelProcessorProvider: pullRequestLabelProcessorProvider, + isPrivileged: isPrivileged, + latestSha: latestSha, + scheduledSha: scheduledSha, + ); + } + + /// Persists the current state to Firestore if modified. + Future persist() async { + if (!_isDirty) { + return; + } + + final documentId = PullRequestState.getDocumentId(slug, prNumber); + final name = + '$kDocumentParent/${PullRequestState.kCollectionId}/$documentId'; + + final state = PullRequestState() + ..slug = slug + ..number = prNumber + ..isPrivileged = _isPrivileged + ..latestSha = latestSha + ..scheduledSha = _scheduledSha; + + final document = Document(name: name, fields: state.fields); + + await firestore.writeViaTransaction(documentsToWrites([document])); + log.info('Persisted PullRequestState for $slug/$prNumber'); + _isDirty = false; + } + + /// The latest SHA that we have processed for this PR. + String get latestSha => _latestSha; + set latestSha(String value) { + if (_latestSha != value) { + _latestSha = value; + _isDirty = true; + } + } + + /// The SHA for which we have scheduled presubmits. + String? get scheduledSha => _scheduledSha; + set scheduledSha(String? value) { + if (_scheduledSha != value) { + _scheduledSha = value; + _isDirty = true; + } + } + + /// Handles PR opened event. + Future _handleOpened(PullRequestEvent event) async { + final pr = event.pullRequest!; + final sha = pr.head!.sha!; + latestSha = sha; // Update latest SHA. + + 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(event); + } else { + await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); + } + + await checkForTests(event); + await _tryReleaseApproval(event); + } + + /// Handles PR synchronize event. + Future _handleSynchronize(PullRequestEvent event) async { + final pr = event.pullRequest!; + final sha = pr.head!.sha!; + latestSha = sha; // Update latest SHA. + + final hasLabel = + pr.labels?.any((IssueLabel l) => l.name == Config.kCicdLabel) ?? false; + final isPrivilegedUser = _isPrivileged; + + if (isPrivilegedUser) { + if (hasLabel) { + await _scheduleIfMergeable(event); + } else { + await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); + } + } else { + if (hasLabel) { + final githubService = await config.createGithubService(slug); + await githubService.removeLabel(slug, pr.number!, Config.kCicdLabel); + log.debug('Removed CICD label from PR ${pr.number}'); + } + await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); + } + } + + /// Handles PR edited event. + Future _handleEdited(PullRequestEvent event) async { + await checkForTests(event); + } + + /// Handles PR reopened event. + Future _handleReopened(PullRequestEvent event) async { + final pr = event.pullRequest!; + await _warnThatANewCommitIsNeeded(event); + await _processLabels(pr); + await _updatePullRequest(pr); + } + + /// Handles PR labeled event. + Future _handleLabeled( + PullRequestEvent event, + String? labelName, + int? labelId, + ) async { + final pr = event.pullRequest!; + final sha = pr.head!.sha!; + + if (labelName == Config.kCicdLabel && + Config.kCicdLabelIds.contains(labelId)) { + log.info('new CICD label added - scheduling tests'); + await scheduler.resolveAwaitingCicdLabelCheckRun(slug, sha); + await _scheduleIfMergeable(event); + } + + await _processLabels(pr); + await _updatePullRequest(pr); + } + + Future checkForTests(PullRequestEvent pullRequestEvent) async { + final pr = pullRequestEvent.pullRequest!; + // We do not need to add test labels if this is an auto roller author. + if (config.rollerAccounts.contains(pr.user!.login)) { + return; + } + final eventAction = pullRequestEvent.action; + final isTipOfTree = pr.base!.ref == Config.defaultBranch(slug); + final gitHubClient = await config.createGitHubClient(pullRequest: pr); + await _validateRefs(gitHubClient, pr); + if (kNeedsTests.contains(slug) && isTipOfTree) { + log.info( + 'Applying framework repo labels for: owner=${slug.owner} repo=${slug.name} and pr=${pr.number}', + ); + switch (slug.name) { + case 'flutter': + final isFusion = slug == Config.flutterSlug; + final files = await gitHubClient.pullRequests + .listFiles(slug, pr.number!) + .toList(); + await _applyFrameworkRepoLabels( + gitHubClient, + eventAction, + pr, + files, + slug, + ); + if (isFusion) { + await _applyEngineRepoLabels( + gitHubClient, + eventAction, + pr, + files, + slug, + ); + } + case 'packages': + return _applyPackageTestChecks(gitHubClient, eventAction, pr); + } + } + } + + Future _applyFrameworkRepoLabels( + GitHub gitHubClient, + String? eventAction, + PullRequest pr, + List files, + RepositorySlug slug, + ) async { + if (pr.user!.login == 'engine-flutter-autoroll') { + return; + } + + final labels = {}; + var hasTests = false; + var needsTests = false; + + var frameworkFiles = 0; + + for (var file in files) { + final filename = file.filename!; + + if (!_isFusionEnginePath(filename)) { + frameworkFiles++; + } + + if (_fileContainsAddedCode(file) && + !_isTestExempt(filename) && + !filename.startsWith('dev/bots/') && + !filename.endsWith('.gitignore')) { + needsTests = !_allChangesAreCodeComments(file); + } + + // Check to see if tests were submitted with this PR. + if (_isAFrameworkTest(filename)) { + hasTests = true; + } + } + + if (frameworkFiles == 0) { + // a fusion / engine only change. + return; + } + + if (pr.user!.login == 'fluttergithubbot') { + needsTests = false; + labels.addAll(['c: tech-debt', 'c: flake']); + } + + if (labels.isNotEmpty) { + await gitHubClient.issues.addLabelsToIssue( + slug, + pr.number!, + labels.toList(), + ); + } + + if (!hasTests && + needsTests && + !pr.draft! && + !_isPrUpdatingReleaseBranch(pr)) { + final body = config.missingTestsPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + } + } + + bool _isAFrameworkTest(String filename) { + if (kNotActuallyATest.any(filename.endsWith)) { + return false; + } + // Check for Objective-C tests which end in either "Tests.m" or "Test.m" + // in the "dev" directory. + final objectiveCTestRegex = RegExp(r'.*dev\/.*Test[s]?\.m$'); + return filename.endsWith('_test.dart') || + filename.endsWith('.expect') || + filename.contains('test_fixes') || + // Include updates to test utilities or test data + filename.contains('packages/flutter_tools/test/') || + // Kotlin source tests, used in the Flutter Gradle Plugin. + filename.startsWith('packages/flutter_tools/gradle/src/test/') || + filename.startsWith('dev/bots/analyze.dart') || + filename.startsWith('dev/bots/test.dart') || + filename.startsWith('dev/devicelab/bin/tasks') || + filename.startsWith('dev/devicelab/lib/tasks') || + filename.startsWith('dev/benchmarks') || + objectiveCTestRegex.hasMatch(filename); + } + + /// Returns true if changes to [filename] are exempt from the testing + /// requirement, across repositories. + bool _isTestExempt(String filename, {String engineBasePath = ''}) { + final isBuildPythonScript = + filename.startsWith('${engineBasePath}sky/tools') && + filename.endsWith('.py'); + return filename.contains('.ci.yaml') || + filename.endsWith('analysis_options.yaml') || + filename.endsWith('AUTHORS') || + filename.endsWith('CODEOWNERS') || + filename.endsWith('TESTOWNERS') || + filename.endsWith('pubspec.yaml') || + filename.endsWith('pubspec.yaml.tmpl') || + // Exempt categories. + filename.contains('.gemini/') || + filename.contains('.github/') || + filename.endsWith('.md') || + // Exempt paths. + filename.startsWith('dev/customer_testing/tests.version') || + filename.startsWith('dev/devicelab/lib/versions/gallery.dart') || + filename.startsWith('dev/integration_tests/') || + filename.startsWith('docs/') || + filename.startsWith('${engineBasePath}docs/') || + filename.endsWith('test/flutter_test_config.dart') || + // ↓↓↓ Begin engine specific paths ↓↓↓ + filename == 'DEPS' || // note: in monorepo; DEPS is still at the root. + isBuildPythonScript || + filename.endsWith('.gni') || + filename.endsWith('.gn') || + filename.startsWith('${engineBasePath}impeller/fixtures/') || + filename.startsWith('${engineBasePath}impeller/golden_tests/') || + filename.startsWith('${engineBasePath}impeller/playground/') || + filename.startsWith( + '${engineBasePath}shell/platform/embedder/tests/', + ) || + filename.startsWith( + '${engineBasePath}shell/platform/embedder/fixtures/', + ) || + filename.startsWith('${engineBasePath}testing/') || + filename.startsWith('${engineBasePath}tools/clangd_check/'); + } + + bool _isFusionEnginePath(String path) => + path.startsWith('engine/') || path == 'DEPS'; + + Future _applyEngineRepoLabels( + GitHub gitHubClient, + String? eventAction, + PullRequest pr, + List files, + RepositorySlug slug, + ) async { + // Do not apply the test labels for the autoroller accounts. + if (pr.user!.login == 'skia-flutter-autoroll') { + return; + } + + var hasTests = false; + var needsTests = false; + + // If engine labels are being applied to the flutterSlug - we're in a fusion repo. + final isFusion = slug == Config.flutterSlug; + final engineBasePath = isFusion ? 'engine/src/flutter/' : ''; + + var engineFiles = 0; + + for (var file in files) { + final path = file.filename!; + if (isFusion && _isFusionEnginePath(path)) { + engineFiles++; + } + + if (_fileContainsAddedCode(file) && + !_isTestExempt(path, engineBasePath: engineBasePath) && + // License goldens are auto-generated. + !path.startsWith('${engineBasePath}ci/licenses_golden/') && + // Build configuration files tell CI what to run. + !path.startsWith('${engineBasePath}ci/builders/')) { + needsTests = !_allChangesAreCodeComments(file); + } + + if (_isAnEngineTest(path)) { + hasTests = true; + } + } + + if (isFusion && engineFiles == 0) { + // framework only change + return; + } + + if (!hasTests && + needsTests && + !pr.draft! && + !_isPrUpdatingReleaseBranch(pr)) { + final body = config.missingTestsPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + } + } + + bool _isAnEngineTest(String filename) { + final engineTestRegExp = RegExp( + r'(tests?|benchmarks?)\.(dart|java|mm|m|cc|sh|py|swift)$', + ); + return filename.contains('IosBenchmarks') || + filename.contains('IosUnitTests') || + filename.contains('scenario_app') || + engineTestRegExp.hasMatch(filename.toLowerCase()); + } + + bool _fileContainsAddedCode(PullRequestFile file) { + // When null, do not assume 0 lines have been added. + final linesAdded = file.additionsCount ?? 1; + final linesDeleted = file.deletionsCount ?? 0; + final linesTotal = file.changesCount ?? linesDeleted + linesAdded; + return linesAdded > 0 || linesDeleted != linesTotal; + } + + // Runs automated test checks for both flutter/packages. + Future _applyPackageTestChecks( + GitHub gitHubClient, + String? eventAction, + PullRequest pr, + ) async { + final slug = pr.base!.repo!.slug(); + final files = gitHubClient.pullRequests.listFiles(slug, pr.number!); + var hasTests = false; + var needsTests = false; + + await for (PullRequestFile file in files) { + final filename = file.filename!; + + if (_fileContainsAddedCode(file) && + !_isTestExempt(filename) && + !filename.contains('.ci/') && + // Custom package-specific test runners. These do not count as tests + // for the purposes of testing a change that otherwise needs tests, + // but since they are the driver for tests they don't need test + // coverage. + !filename.endsWith('tool/run_tests.dart') && + !filename.endsWith('run_tests.sh')) { + needsTests = !_allChangesAreCodeComments(file); + } + // See https://github.com/flutter/flutter/blob/master/docs/ecosystem/testing/Plugin-Tests.md for discussion + // of various plugin test types and locations. + if (filename.endsWith('_test.dart') || + // Native iOS/macOS tests. + filename.contains('RunnerTests/') || + filename.contains('RunnerUITests/') || + filename.contains('darwin/Tests/') || + // Native Android tests. + filename.contains('android/src/test/') || + filename.contains('androidTest/') || + // Native Linux tests. + filename.endsWith('_test.cc') || + // Native Windows tests. + filename.endsWith('_test.cpp') || + // Pigeon native tests. + filename.contains('/platform_tests/') || + // Test files in package-specific test folders. + filename.contains('go_router/test_fixes/') || + filename.contains('go_router_builder/test_inputs/')) { + hasTests = true; + } + } + + if (!hasTests && + needsTests && + !pr.draft! && + !_isPrUpdatingReleaseBranch(pr)) { + final body = config.missingTestsPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + } + } + + /// Validate the base and head refs of the PR. + Future _validateRefs(GitHub gitHubClient, PullRequest pr) async { + final slug = pr.base!.repo!.slug(); + String body; + const releaseChannels = ['stable', 'beta', 'dev']; + // Close PRs that use a release branch as a source. + if (releaseChannels.contains(pr.head!.ref)) { + body = config.wrongHeadBranchPullRequestMessage(pr.head!.ref!); + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.pullRequests.edit(slug, pr.number!, state: 'closed'); + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + return; + } + final defaultBranchName = Config.defaultBranch(pr.base!.repo!.slug()); + final baseName = pr.base!.ref!; + if (baseName == defaultBranchName) { + return; + } + if (_isPrUpdatingReleaseBranch(pr)) { + body = config.releaseBranchPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + return; + } + + // For repos migrated to main, close PRs opened against master. + final isMaster = pr.base?.ref == 'master'; + final isMigrated = defaultBranchName == 'main'; + // PRs should never be open to "beta" or "stable." + final isReleaseChannelBranch = releaseChannels.contains(pr.base?.ref); + if ((isMaster && isMigrated) || isReleaseChannelBranch) { + body = _getWrongBaseComment( + base: baseName, + defaultBranch: defaultBranchName, + ); + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.pullRequests.edit( + slug, + pr.number!, + base: Config.defaultBranch(slug), + ); + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + } + } + + static bool _isPrUpdatingReleaseBranch(PullRequest pr) { + return isReleaseCandidateBranch(branchName: pr.base!.ref!); + } + + Future _alreadyCommented( + GitHub gitHubClient, + PullRequest pr, + String message, + ) async { + final comments = gitHubClient.issues.listCommentsByIssue( + pr.base!.repo!.slug(), + pr.number!, + ); + await for (IssueComment comment in comments) { + if (comment.body != null && comment.body!.contains(message)) { + return true; + } + } + return false; + } + + String _getWrongBaseComment({ + required String base, + required String defaultBranch, + }) { + final messageTemplate = config.wrongBaseBranchPullRequestMessage; + return messageTemplate + .replaceAll('{{target_branch}}', base) + .replaceAll('{{default_branch}}', defaultBranch); + } + + /// Returns true if the changes to [file] are all code comments. + /// + /// If that cannot be determined with confidence, returns false. False + /// negatives (e.g., for /* */-style multi-line comments) should be expected. + bool _allChangesAreCodeComments(PullRequestFile file) { + final linesAdded = file.additionsCount; + final linesDeleted = file.deletionsCount; + final patch = file.patch; + // If information is missing, err or the side of assuming it's a non-comment + // change. + if (linesAdded == null || linesDeleted == null || patch == null) { + return false; + } + + final filename = file.filename!; + final extension = filename.contains('.') + ? filename.split('.').last.toLowerCase() + : null; + if (extension == null || !knownCommentCodeExtensions.contains(extension)) { + return false; + } + + // Only handles single-line comments; identifying multi-line comments + // would require the full file and non-trivial parsing. Also doesn't handle + // end-of-line comments (e.g., "int x = 0; // Info about x"). + final commentRegex = RegExp(r'^[+-]\s*//'); + final onlyWhitespaceRegex = RegExp(r'^[+-]\s*$'); + for (var line in patch.split('\n')) { + if (!line.startsWith('+') && !line.startsWith('-')) { + continue; + } + + if (onlyWhitespaceRegex.hasMatch(line)) { + // whitespace only changes don't require tests + continue; + } + + if (!commentRegex.hasMatch(line)) { + return false; + } + } + return true; + } + + /// Release tooling generates cherrypick pull requests that should be granted an approval. + Future _tryReleaseApproval(PullRequestEvent pullRequestEvent) async { + final pr = pullRequestEvent.pullRequest!; + final slug = pullRequestEvent.repository!.slug(); + + final defaultBranch = Config.defaultBranch(slug); + final branch = pr.base?.ref; + if (branch == null || branch.contains(defaultBranch)) { + // This isn't a release branch PR + return; + } + + final releaseAccounts = await config.releaseAccounts; + if (releaseAccounts.contains(pr.user?.login) == false) { + // The author isn't in the list of release accounts, do nothing + return; + } + + final gitHubClient = config.createGitHubClientWithToken( + await config.githubOAuthToken, + ); + final review = CreatePullRequestReview( + slug.owner, + slug.name, + pr.number!, + 'APPROVE', + ); + await gitHubClient.pullRequests.createReview(slug, review); + } + + Future _processLabels(PullRequest pullRequest) async { + final slug = pullRequest.base!.repo!.slug(); + final githubService = await config.createGithubService(slug); + + final labelProcessor = pullRequestLabelProcessorProvider( + config: config, + githubService: githubService, + pullRequest: pullRequest, + ); + + return labelProcessor.processLabels(); + } + + Future _scheduleIfMergeable(PullRequestEvent pullRequestEvent) async { + final pr = pullRequestEvent.pullRequest!; + final slug = pullRequestEvent.repository!.slug(); + final sha = pr.head!.sha!; + + if (scheduledSha == sha) { + log.info('Presubmits already scheduled for SHA $sha. Skipping.'); + return; + } + + log.info( + 'Scheduling tasks if mergeable(${pr.mergeable}): owner=${slug.owner} repo=${slug.name} and pr=${pr.number}', + ); + + if (pr.mergeable == false) { + final slug = pullRequestEvent.repository!.slug(); + final gitHubClient = await config.createGitHubClient(pullRequest: pr); + final body = config.mergeConflictPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + return; + } + + try { + await scheduler.triggerPresubmitTargets(pullRequest: pr); + scheduledSha = sha; // Update scheduled SHA! + await _processLabels(pr); + } on DetailedApiRequestError catch (e) { + if (e.status != HttpStatus.conflict) { + rethrow; + } + await _warnThatANewCommitIsNeeded(pullRequestEvent); + } + } + + Future _warnThatANewCommitIsNeeded( + PullRequestEvent pullRequestEvent, + ) async { + final pr = pullRequestEvent.pullRequest!; + final slug = pullRequestEvent.repository!.slug(); + final sha = pr.head!.sha!; + + final gitHubClient = config.createGitHubClientWithToken( + await config.githubOAuthToken, + ); + await gitHubClient.issues.createComment( + slug, + pr.number!, + Config.newCommitIsNeeded(sha: sha), + ); + } + + /// Update the PR stored in [PrCheckRuns] so that subsequent checks are fresh. + Future _updatePullRequest(PullRequest pr) async { + final sha = pr.head!.sha!; + final didUpdate = await PrCheckRuns.updatePullRequestForSha( + firestore, + sha, + pr, + ); + if (!didUpdate) { + log.debug('No PR found for SHA: $sha, did not update'); + } else { + log.debug('Updated PR for SHA: $sha'); + } + } +} + +class PullRequestEventContext { + final CacheService cache; + final FirestoreService firestore; + final Config config; + final Scheduler scheduler; + final GerritService gerritService; + final PullRequestLabelProcessorProvider pullRequestLabelProcessorProvider; + + PullRequestEventContext({ + required this.cache, + required this.firestore, + required this.config, + required this.scheduler, + required this.gerritService, + required this.pullRequestLabelProcessorProvider, + }); +} + +typedef PullRequestLabelProcessorProvider = + PullRequestLabelProcessor Function({ + required Config config, + required GithubService githubService, + required PullRequest pullRequest, + }); + +class PullRequestLabelProcessor { + PullRequestLabelProcessor({ + required this.config, + required this.githubService, + required this.pullRequest, + }) : slug = pullRequest.base!.repo!.slug(), + prNumber = pullRequest.number!; + + static const kEmergencyLabelEducation = ''' +Detected the `emergency` label. + +If you add the `autosubmit` label, the bot will wait until all presubmits pass but ignore the tree status, allowing fixes for tree breakages while still validating that they don't break any existing presubmits. + +The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". +'''; + + final Config config; + final GithubService githubService; + final PullRequest pullRequest; + final RepositorySlug slug; + final int prNumber; + late final String logCrumb = + '$PullRequestLabelProcessor($slug/pull/$prNumber)'; + + void logInfo(Object? message) { + log.info('$logCrumb: $message'); + } + + void logSevere(Object? message, {Object? error, StackTrace? stackTrace}) { + log.error('$logCrumb: $message', error, stackTrace); + } + + Future processLabels() async { + final hasEmergencyLabel = + pullRequest.labels?.any( + (label) => label.name == Config.kEmergencyLabel, + ) ?? + false; + if (hasEmergencyLabel) { + // The merge guard and Flutter Presubmits check can be unlocked without approval checks because: + // + // * For manual merges the GitHub repo settings already require minimum + // approvals before the PR can be submitted. + // * For `autosubmit` label Cocoon has the [Approval] validation that + // checks approvals before attempting to merge the PR. + await _unlockCheckrunsForEmergency(); + } else { + logInfo('no emergency label; moving on.'); + } + } + + Future _unlockCheckrunsForEmergency() async { + await _unlockCheckrun(Config.kMergeQueueLockName); + await _unlockCheckrun(Config.kFlutterPresubmitsName); + + // Let the developer know what is happening with the MQ when this label is found the first time. + try { + if (!await githubService.commentExists( + slug, + prNumber, + PullRequestLabelProcessor.kEmergencyLabelEducation, + )) { + await githubService.createComment( + slug, + issueNumber: prNumber, + body: PullRequestLabelProcessor.kEmergencyLabelEducation, + ); + } + } catch (e, s) { + logSevere( + 'failed to leave educational comment for emergency label.', + error: e, + stackTrace: s, + ); + } + } + + Future _unlockCheckrun(String checkName) async { + logInfo('attempting to unlock the $checkName for emergency'); + + final guard = (await githubService.getCheckRunsFiltered( + slug: slug, + ref: pullRequest.head!.sha!, + checkName: checkName, + )).singleOrNull; + + if (guard == null) { + logSevere( + 'failed to process the emergency label. "$checkName" check run is missing.', + ); + return; + } + + await githubService.updateCheckRun( + slug: slug, + checkRun: guard, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + output: CheckRunOutput( + title: checkName, + summary: 'Emergency label applied.', + ), + ); + + logInfo('unlocked "$checkName", allowing it to land as an emergency.'); + } +} 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 fc09e7ff5..27b32ced9 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -4,6 +4,7 @@ import 'dart:async'; import 'dart:io'; +import 'dart:typed_data'; import 'package:buildbucket/buildbucket_pb.dart' as bbv2; import 'package:cocoon_common/core_extensions.dart'; @@ -23,6 +24,7 @@ import 'package:cocoon_service/src/service/big_query.dart'; import 'package:cocoon_service/src/service/cache_service.dart'; import 'package:cocoon_service/src/service/config.dart'; import 'package:cocoon_service/src/service/github_service.dart'; +import 'package:cocoon_service/src/service/pull_request_manager.dart'; import 'package:cocoon_service/src/service/scheduler.dart'; import 'package:fixnum/fixnum.dart'; import 'package:github/github.dart' hide Branch; @@ -46,6 +48,7 @@ void main() { useTestLoggerPerTest(); late GithubWebhookSubscription webhook; + late CacheService cache; late FakeBuildBucketClient fakeBuildBucketClient; late FakeConfig config; late FakeGithubService githubService; @@ -162,9 +165,10 @@ void main() { gerritService = FakeGerritService(); fakeNow = DateTime.now(); + cache = CacheService(inMemory: true); webhook = GithubWebhookSubscription( config: config, - cache: CacheService(inMemory: true), + cache: cache, gerritService: gerritService, scheduler: scheduler, commitService: commitService, @@ -3949,50 +3953,96 @@ 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'); + // Setup: Open PR as privileged user to populate state + tester.message = generateGithubWebhookMessage( + action: 'opened', + login: 'test-flutter-hacker', + ); + await tester.post(webhook); + expect(scheduler.triggerPresubmitTargetsCnt, 1); + + // Act: Synchronize with label + 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, ); @@ -4007,16 +4057,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); @@ -4034,12 +4085,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); @@ -4096,4 +4147,29 @@ void foo() { }, ); }); + + group('Redis locks', () { + test('fails with 503 when PR is locked', () async { + final slug = Config.flutterSlug; + const prNumber = 123; + final lockKey = 'pr_lock_${slug.owner}_${slug.name}_$prNumber'; + final lockValue = Uint8List.fromList('l'.codeUnits); + + // Pre-populate cache to simulate lock held by another instance + await cache.set('pr_locks', lockKey, lockValue); + + tester.message = generateGithubWebhookMessage( + action: 'opened', + number: prNumber, + ); + + try { + await tester.post(webhook); + fail('Should have thrown ServiceUnavailable'); + } on ServiceUnavailable catch (e) { + expect(e.statusCode, HttpStatus.serviceUnavailable); + expect(e.message, contains('PR is locked by another instance')); + } + }); + }); } diff --git a/packages/cocoon_integration_test/lib/src/utilities/mocks.dart b/packages/cocoon_integration_test/lib/src/utilities/mocks.dart index a5ccfc38a..67878c7d3 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.dart @@ -6,7 +6,6 @@ import 'dart:io'; import 'dart:typed_data'; import 'package:cocoon_service/src/foundation/github_checks_util.dart'; -import 'package:cocoon_service/src/request_handlers/github/webhook_subscription.dart'; import 'package:cocoon_service/src/service/access_token_provider.dart'; import 'package:cocoon_service/src/service/big_query.dart'; import 'package:cocoon_service/src/service/branch_service.dart'; @@ -17,6 +16,7 @@ import 'package:cocoon_service/src/service/discord_service.dart'; import 'package:cocoon_service/src/service/github_checks_service.dart'; import 'package:cocoon_service/src/service/github_service.dart'; import 'package:cocoon_service/src/service/luci_build_service.dart'; +import 'package:cocoon_service/src/service/pull_request_manager.dart'; import 'package:cocoon_service/src/service/scheduler.dart'; import 'package:github/github.dart'; import 'package:googleapis/bigquery/v2.dart';