[flutter_plugin_tools] Add a federated PR safety check (#4329)

Creates a new command to validate that PRs don't change platform interface packages and implementations at the same time, to try to prevent ecosystem-breaking changes. See https://github.com/flutter/flutter/issues/89518 for context.

Per the explanation in the issue, this has carve-outs for:
- Changes to platform interfaces that aren't published (allowing for past uses cases such as making a substantive change to an implementation, and making minor adjustments to comments in the PI package based on those changes).
- Things that look like bulk changes (e.g., a mass change to account for a new lint rule)

Fixes https://github.com/flutter/flutter/issues/89518

script/tool/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 - `native-test --android`, `--ios`, and `--macos` now fail plugins that don't
   have unit tests, rather than skipping them.
+- Added a new `federation-safety-check` command to help catch changes to
+  federated packages that have been done in such a way that they will pass in
+  CI, but fail once the change is landed and published.
 
 ## 0.7.1
 

script/tool/lib/src/common/git_version_finder.dart

@@ -58,6 +58,9 @@ class GitVersionFinder {
       return null;
     }
     final String fileContent = gitShow.stdout as String;
+    if (fileContent.trim().isEmpty) {
+      return null;
+    }
     final String? versionString = loadYaml(fileContent)['version'] as String?;
     return versionString == null ? null : Version.parse(versionString);
   }

script/tool/lib/src/common/repository_package.dart

@@ -47,6 +47,12 @@ class RepositoryPackage {
   /// The package's top-level pubspec.yaml.
   File get pubspecFile => directory.childFile('pubspec.yaml');
 
+  /// True if this appears to be a federated plugin package, according to
+  /// repository conventions.
+  bool get isFederated =>
+      directory.parent.basename != 'packages' &&
+      directory.basename.startsWith(directory.parent.basename);
+
   /// Returns the Flutter example packages contained in the package, if any.
   Iterable<RepositoryPackage> getExamples() {
     final Directory exampleDirectory = directory.childDirectory('example');

script/tool/lib/src/federation_safety_check_command.dart

@@ -0,0 +1,188 @@
+// Copyright 2013 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 'package:file/file.dart';
+import 'package:flutter_plugin_tools/src/common/plugin_utils.dart';
+import 'package:git/git.dart';
+import 'package:path/path.dart' as p;
+import 'package:platform/platform.dart';
+import 'package:pub_semver/pub_semver.dart';
+import 'package:pubspec_parse/pubspec_parse.dart';
+
+import 'common/core.dart';
+import 'common/file_utils.dart';
+import 'common/git_version_finder.dart';
+import 'common/package_looping_command.dart';
+import 'common/process_runner.dart';
+import 'common/repository_package.dart';
+
+/// A command to check that PRs don't violate repository best practices that
+/// have been established to avoid breakages that building and testing won't
+/// catch.
+class FederationSafetyCheckCommand extends PackageLoopingCommand {
+  /// Creates an instance of the safety check command.
+  FederationSafetyCheckCommand(
+    Directory packagesDir, {
+    ProcessRunner processRunner = const ProcessRunner(),
+    Platform platform = const LocalPlatform(),
+    GitDir? gitDir,
+  }) : super(
+          packagesDir,
+          processRunner: processRunner,
+          platform: platform,
+          gitDir: gitDir,
+        );
+
+  // A map of package name (as defined by the directory name of the package)
+  // to a list of changed Dart files in that package, as Posix paths relative to
+  // the package root.
+  //
+  // This only considers top-level packages, not subpackages such as example/.
+  final Map<String, List<String>> _changedDartFiles = <String, List<String>>{};
+
+  // The set of *_platform_interface packages that will have public code changes
+  // published.
+  final Set<String> _modifiedAndPublishedPlatformInterfacePackages = <String>{};
+
+  // The set of conceptual plugins (not packages) that have changes.
+  final Set<String> _changedPlugins = <String>{};
+
+  static const String _platformInterfaceSuffix = '_platform_interface';
+
+  @override
+  final String name = 'federation-safety-check';
+
+  @override
+  final String description =
+      'Checks that the change does not violate repository rules around changes '
+      'to federated plugin packages.';
+
+  @override
+  bool get hasLongOutput => false;
+
+  @override
+  Future<void> initializeRun() async {
+    final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
+    final String baseSha = await gitVersionFinder.getBaseSha();
+    print('Validating changes relative to "$baseSha"\n');
+    for (final String path in await gitVersionFinder.getChangedFiles()) {
+      // Git output always uses Posix paths.
+      final List<String> allComponents = p.posix.split(path);
+      final int packageIndex = allComponents.indexOf('packages');
+      if (packageIndex == -1) {
+        continue;
+      }
+      final List<String> relativeComponents =
+          allComponents.sublist(packageIndex + 1);
+      // The package name is either the directory directly under packages/, or
+      // the directory under that in the case of a federated plugin.
+      String packageName = relativeComponents.removeAt(0);
+      // Count the top-level plugin as changed.
+      _changedPlugins.add(packageName);
+      if (relativeComponents[0] == packageName ||
+          relativeComponents[0].startsWith('${packageName}_')) {
+        packageName = relativeComponents.removeAt(0);
+      }
+
+      if (relativeComponents.last.endsWith('.dart')) {
+        _changedDartFiles[packageName] ??= <String>[];
+        _changedDartFiles[packageName]!
+            .add(p.posix.joinAll(relativeComponents));
+      }
+
+      if (packageName.endsWith(_platformInterfaceSuffix) &&
+          relativeComponents.first == 'pubspec.yaml' &&
+          await _packageWillBePublished(path)) {
+        _modifiedAndPublishedPlatformInterfacePackages.add(packageName);
+      }
+    }
+  }
+
+  @override
+  Future<PackageResult> runForPackage(RepositoryPackage package) async {
+    if (!isFlutterPlugin(package)) {
+      return PackageResult.skip('Not a plugin.');
+    }
+
+    if (!package.isFederated) {
+      return PackageResult.skip('Not a federated plugin.');
+    }
+
+    if (package.directory.basename.endsWith('_platform_interface')) {
+      // As the leaf nodes in the graph, a published package interface change is
+      // assumed to be correct, and other changes are validated against that.
+      return PackageResult.skip(
+          'Platform interface changes are not validated.');
+    }
+
+    // Uses basename to match _changedPackageFiles.
+    final String basePackageName = package.directory.parent.basename;
+    final String platformInterfacePackageName =
+        '$basePackageName$_platformInterfaceSuffix';
+    final List<String> changedPlatformInterfaceFiles =
+        _changedDartFiles[platformInterfacePackageName] ?? <String>[];
+
+    if (!_modifiedAndPublishedPlatformInterfacePackages
+        .contains(platformInterfacePackageName)) {
+      print('No published changes for $platformInterfacePackageName.');
+      return PackageResult.success();
+    }
+
+    if (!changedPlatformInterfaceFiles
+        .any((String path) => path.startsWith('lib/'))) {
+      print('No public code changes for $platformInterfacePackageName.');
+      return PackageResult.success();
+    }
+
+    // If the change would be flagged, but it appears to be a mass change
+    // rather than a plugin-specific change, allow it with a warning.
+    //
+    // This is a tradeoff between safety and convenience; forcing mass changes
+    // to be split apart is not ideal, and the assumption is that reviewers are
+    // unlikely to accidentally approve a PR that is supposed to be changing a
+    // single plugin, but touches other plugins (vs accidentally approving a
+    // PR that changes multiple parts of a single plugin, which is a relatively
+    // easy mistake to make).
+    //
+    // 3 is chosen to minimize the chances of accidentally letting something
+    // through (vs 2, which could be a single-plugin change with one stray
+    // change to another file accidentally included), while not setting too
+    // high a bar for detecting mass changes. This can be tuned if there are
+    // issues with false positives or false negatives.
+    const int massChangePluginThreshold = 3;
+    if (_changedPlugins.length >= massChangePluginThreshold) {
+      logWarning('Ignoring potentially dangerous change, as this appears '
+          'to be a mass change.');
+      return PackageResult.success();
+    }
+
+    printError('Dart changes are not allowed to other packages in '
+        '$basePackageName in the same PR as changes to public Dart code in '
+        '$platformInterfacePackageName, as this can cause accidental breaking '
+        'changes to be missed by automated checks. Please split the changes to '
+        'these two packages into separate PRs.\n\n'
+        'If you believe that this is a false positive, please file a bug.');
+    return PackageResult.fail(
+        <String>['$platformInterfacePackageName changed.']);
+  }
+
+  Future<bool> _packageWillBePublished(
+      String pubspecRepoRelativePosixPath) async {
+    final File pubspecFile = childFileWithSubcomponents(
+        packagesDir.parent, p.posix.split(pubspecRepoRelativePosixPath));
+    final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync());
+    if (pubspec.publishTo == 'none') {
+      return false;
+    }
+
+    final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
+    final Version? previousVersion =
+        await gitVersionFinder.getPackageVersion(pubspecRepoRelativePosixPath);
+    if (previousVersion == null) {
+      // The plugin is new, so it will be published.
+      return true;
+    }
+    return pubspec.version != previousVersion;
+  }
+}

script/tool/lib/src/main.dart

@@ -13,6 +13,7 @@ import 'build_examples_command.dart';
 import 'common/core.dart';
 import 'create_all_plugins_app_command.dart';
 import 'drive_examples_command.dart';
+import 'federation_safety_check_command.dart';
 import 'firebase_test_lab_command.dart';
 import 'format_command.dart';
 import 'license_check_command.dart';
@@ -49,6 +50,7 @@ void main(List<String> args) {
     ..addCommand(BuildExamplesCommand(packagesDir))
     ..addCommand(CreateAllPluginsAppCommand(packagesDir))
     ..addCommand(DriveExamplesCommand(packagesDir))
+    ..addCommand(FederationSafetyCheckCommand(packagesDir))
     ..addCommand(FirebaseTestLabCommand(packagesDir))
     ..addCommand(FormatCommand(packagesDir))
     ..addCommand(LicenseCheckCommand(packagesDir))

script/tool/lib/src/pubspec_check_command.dart

@@ -210,17 +210,11 @@ class PubspecCheckCommand extends PackageLoopingCommand {
   // Returns true if [packageName] appears to be an implementation package
   // according to repository conventions.
   bool _isImplementationPackage(RepositoryPackage package) {
-    // An implementation package should be in a group folder...
-    final Directory parentDir = package.directory.parent;
-    if (parentDir.path == packagesDir.path) {
+    if (!package.isFederated) {
       return false;
     }
     final String packageName = package.directory.basename;
-    final String parentName = parentDir.basename;
-    // ... whose name is a prefix of the package name.
-    if (!packageName.startsWith(parentName)) {
-      return false;
-    }
+    final String parentName = package.directory.parent.basename;
     // A few known package names are not implementation packages; assume
     // anything else is. (This is done instead of listing known implementation
     // suffixes to allow for non-standard suffixes; e.g., to put several

script/tool/test/federation_safety_check_command_test.dart

@@ -0,0 +1,314 @@
+// Copyright 2013 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:io' as io;
+
+import 'package:args/command_runner.dart';
+import 'package:file/file.dart';
+import 'package:file/memory.dart';
+import 'package:flutter_plugin_tools/src/common/core.dart';
+import 'package:flutter_plugin_tools/src/common/repository_package.dart';
+import 'package:flutter_plugin_tools/src/federation_safety_check_command.dart';
+import 'package:mockito/mockito.dart';
+import 'package:test/test.dart';
+
+import 'common/plugin_command_test.mocks.dart';
+import 'mocks.dart';
+import 'util.dart';
+
+void main() {
+  FileSystem fileSystem;
+  late MockPlatform mockPlatform;
+  late Directory packagesDir;
+  late CommandRunner<void> runner;
+  late RecordingProcessRunner processRunner;
+
+  setUp(() {
+    fileSystem = MemoryFileSystem();
+    mockPlatform = MockPlatform();
+    packagesDir = createPackagesDirectory(fileSystem: fileSystem);
+
+    final MockGitDir gitDir = MockGitDir();
+    when(gitDir.path).thenReturn(packagesDir.parent.path);
+    when(gitDir.runCommand(any, throwOnError: anyNamed('throwOnError')))
+        .thenAnswer((Invocation invocation) {
+      final List<String> arguments =
+          invocation.positionalArguments[0]! as List<String>;
+      // Route git calls through the process runner, to make mock output
+      // consistent with other processes. Attach the first argument to the
+      // command to make targeting the mock results easier.
+      final String gitCommand = arguments.removeAt(0);
+      return processRunner.run('git-$gitCommand', arguments);
+    });
+
+    processRunner = RecordingProcessRunner();
+    final FederationSafetyCheckCommand command = FederationSafetyCheckCommand(
+        packagesDir,
+        processRunner: processRunner,
+        platform: mockPlatform,
+        gitDir: gitDir);
+
+    runner = CommandRunner<void>('federation_safety_check_command',
+        'Test for $FederationSafetyCheckCommand');
+    runner.addCommand(command);
+  });
+
+  test('skips non-plugin packages', () async {
+    final Directory package = createFakePackage('foo', packagesDir);
+
+    final String changedFileOutput = <File>[
+      package.childDirectory('lib').childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+    processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+      MockProcess(stdout: changedFileOutput),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo...'),
+        contains('Not a plugin'),
+        contains('Skipped 1 package(s)'),
+      ]),
+    );
+  });
+
+  test('skips unfederated plugins', () async {
+    final Directory package = createFakePlugin('foo', packagesDir);
+
+    final String changedFileOutput = <File>[
+      package.childDirectory('lib').childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+    processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+      MockProcess(stdout: changedFileOutput),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo...'),
+        contains('Not a federated plugin'),
+        contains('Skipped 1 package(s)'),
+      ]),
+    );
+  });
+
+  test('skips interface packages', () async {
+    final Directory pluginGroupDir = packagesDir.childDirectory('foo');
+    final Directory platformInterface =
+        createFakePlugin('foo_platform_interface', pluginGroupDir);
+
+    final String changedFileOutput = <File>[
+      platformInterface.childDirectory('lib').childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+    processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+      MockProcess(stdout: changedFileOutput),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo_platform_interface...'),
+        contains('Platform interface changes are not validated.'),
+        contains('Skipped 1 package(s)'),
+      ]),
+    );
+  });
+
+  test('allows changes to multiple non-interface packages', () async {
+    final Directory pluginGroupDir = packagesDir.childDirectory('foo');
+    final Directory appFacing = createFakePlugin('foo', pluginGroupDir);
+    final Directory implementation =
+        createFakePlugin('foo_bar', pluginGroupDir);
+    createFakePlugin('foo_platform_interface', pluginGroupDir);
+
+    final String changedFileOutput = <File>[
+      appFacing.childFile('foo.dart'),
+      implementation.childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+    processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+      MockProcess(stdout: changedFileOutput),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo/foo...'),
+        contains('No published changes for foo_platform_interface.'),
+        contains('Running for foo_bar...'),
+        contains('No published changes for foo_platform_interface.'),
+      ]),
+    );
+  });
+
+  test(
+      'fails on changes to interface and non-interface packages in the same plugin',
+      () async {
+    final Directory pluginGroupDir = packagesDir.childDirectory('foo');
+    final Directory appFacing = createFakePlugin('foo', pluginGroupDir);
+    final Directory implementation =
+        createFakePlugin('foo_bar', pluginGroupDir);
+    final Directory platformInterface =
+        createFakePlugin('foo_platform_interface', pluginGroupDir);
+
+    final String changedFileOutput = <File>[
+      appFacing.childFile('foo.dart'),
+      implementation.childFile('foo.dart'),
+      platformInterface.childFile('pubspec.yaml'),
+      platformInterface.childDirectory('lib').childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+    processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+      MockProcess(stdout: changedFileOutput),
+    ];
+
+    Error? commandError;
+    final List<String> output = await runCapturingPrint(
+        runner, <String>['federation-safety-check'], errorHandler: (Error e) {
+      commandError = e;
+    });
+
+    expect(commandError, isA<ToolExit>());
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo/foo...'),
+        contains('Dart changes are not allowed to other packages in foo in the '
+            'same PR as changes to public Dart code in foo_platform_interface, '
+            'as this can cause accidental breaking changes to be missed by '
+            'automated checks. Please split the changes to these two packages '
+            'into separate PRs.'),
+        contains('Running for foo_bar...'),
+        contains('Dart changes are not allowed to other packages in foo'),
+        contains('The following packages had errors:'),
+        contains('foo/foo:\n'
+            '    foo_platform_interface changed.'),
+        contains('foo_bar:\n'
+            '    foo_platform_interface changed.'),
+      ]),
+    );
+  });
+
+  test('ignores test-only changes to interface packages', () async {
+    final Directory pluginGroupDir = packagesDir.childDirectory('foo');
+    final Directory appFacing = createFakePlugin('foo', pluginGroupDir);
+    final Directory implementation =
+        createFakePlugin('foo_bar', pluginGroupDir);
+    final Directory platformInterface =
+        createFakePlugin('foo_platform_interface', pluginGroupDir);
+
+    final String changedFileOutput = <File>[
+      appFacing.childFile('foo.dart'),
+      implementation.childFile('foo.dart'),
+      platformInterface.childFile('pubspec.yaml'),
+      platformInterface.childDirectory('test').childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+    processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+      MockProcess(stdout: changedFileOutput),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo/foo...'),
+        contains('No public code changes for foo_platform_interface.'),
+        contains('Running for foo_bar...'),
+        contains('No public code changes for foo_platform_interface.'),
+      ]),
+    );
+  });
+
+  test('ignores unpublished changes to interface packages', () async {
+    final Directory pluginGroupDir = packagesDir.childDirectory('foo');
+    final Directory appFacing = createFakePlugin('foo', pluginGroupDir);
+    final Directory implementation =
+        createFakePlugin('foo_bar', pluginGroupDir);
+    final Directory platformInterface =
+        createFakePlugin('foo_platform_interface', pluginGroupDir);
+
+    final String changedFileOutput = <File>[
+      appFacing.childFile('foo.dart'),
+      implementation.childFile('foo.dart'),
+      platformInterface.childFile('pubspec.yaml'),
+      platformInterface.childDirectory('lib').childFile('foo.dart'),
+    ].map((File file) => file.path).join('\n');
+    processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+      MockProcess(stdout: changedFileOutput),
+    ];
+    // Simulate no change to the version in the interface's pubspec.yaml.
+    processRunner.mockProcessesForExecutable['git-show'] = <io.Process>[
+      MockProcess(
+          stdout: RepositoryPackage(platformInterface)
+              .pubspecFile
+              .readAsStringSync()),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo/foo...'),
+        contains('No published changes for foo_platform_interface.'),
+        contains('Running for foo_bar...'),
+        contains('No published changes for foo_platform_interface.'),
+      ]),
+    );
+  });
+
+  test('allows things that look like mass changes, with warning', () async {
+    final Directory pluginGroupDir = packagesDir.childDirectory('foo');
+    final Directory appFacing = createFakePlugin('foo', pluginGroupDir);
+    final Directory implementation =
+        createFakePlugin('foo_bar', pluginGroupDir);
+    final Directory platformInterface =
+        createFakePlugin('foo_platform_interface', pluginGroupDir);
+
+    final Directory otherPlugin1 = createFakePlugin('bar', packagesDir);
+    final Directory otherPlugin2 = createFakePlugin('baz', packagesDir);
+
+    final String changedFileOutput = <File>[
+      appFacing.childFile('foo.dart'),
+      implementation.childFile('foo.dart'),
+      platformInterface.childFile('pubspec.yaml'),
+      platformInterface.childDirectory('lib').childFile('foo.dart'),
+      otherPlugin1.childFile('bar.dart'),
+      otherPlugin2.childFile('baz.dart'),
+    ].map((File file) => file.path).join('\n');
+    processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+      MockProcess(stdout: changedFileOutput),
+    ];
+
+    final List<String> output =
+        await runCapturingPrint(runner, <String>['federation-safety-check']);
+
+    expect(
+      output,
+      containsAllInOrder(<Matcher>[
+        contains('Running for foo/foo...'),
+        contains(
+            'Ignoring potentially dangerous change, as this appears to be a mass change.'),
+        contains('Running for foo_bar...'),
+        contains(
+            'Ignoring potentially dangerous change, as this appears to be a mass change.'),
+        contains('Ran for 2 package(s) (2 with warnings)'),
+      ]),
+    );
+  });
+}