This validation logic can be made more robust. The current implementation uses as casts which can throw a TypeError if the YAML structure is not as expected (e.g., if release is not a map). Also, a YamlException from loadYaml would be caught as a generic exception higher up. It's better to handle these cases gracefully with type checks and a specific try-catch for YamlException to provide clearer error messages.

    final YamlMap config;
    try {
      final dynamic yaml = loadYaml(ciConfig.readAsStringSync());
      if (yaml is! YamlMap) {
        printError('${indentation}The ci_config.yaml file must be a map.');
        errors.add('Root of config is not a map.');
        return errors;
      }
      config = yaml;
    } on YamlException catch (e) {
      printError(
          '${indentation}Invalid YAML in ${getRelativePosixPath(ciConfig, from: mainPackage.directory)}:');
      printError(e.toString());
      errors.add('Invalid YAML');
      return errors;
    }

    final Object? releaseNode = config['release'];
    if (releaseNode == null) {
      printError('${indentation}Must have `release` property in '
          '${getRelativePosixPath(ciConfig, from: mainPackage.directory)}');
      errors.add('Missing `release` property');
    } else if (releaseNode is! YamlMap) {
      printError('${indentation}The `release` property must be a map in '
          '${getRelativePosixPath(ciConfig, from: mainPackage.directory)}');
      errors.add('`release` property is not a map');
    } else {
      final YamlMap release = releaseNode;
      final Object? batch = release['batch'];
      if (batch == null) {
        printError('${indentation}Must have `batch` property in `release` in '
            '${getRelativePosixPath(ciConfig, from: mainPackage.directory)}');
        errors.add('Missing `batch` property in `release`');
      } else if (batch is! bool) {
        printError(
            '${indentation}The `batch` property in `release` must be boolean in '
            '${getRelativePosixPath(ciConfig, from: mainPackage.directory)}');
        errors.add('Wrong value for `batch`');
      }
    }

There's a TODO here to update a link. This should be resolved before merging, or a tracking issue should be created to ensure it's not forgotten.

What's the reason for requiring every package to explicitly configure everything? I was envisioning this being more like a Dart test config file, where a package only needs a file if it needs non-default behavior.

Requiring every package to explicitly declare everything would make this a maintenance nightmare, because every time we add a new config option we would need to touch every single package.

Validation can go into RepoPackageInfoCheckCommand as a new method, rather than requiring a whole new command.

I don't think we should require every file to have this key, for the same reason that I don't think we should require every package to have the file. We should have reasonable defaults, and only make people opt in to unusual behavior.

+1

Originally I did this for discoverability and make it more clear what the behavior is instead giving a default when not provided.

Without this I would have to document the default behavior.

but I am ok changing it

Without this I would have to document the default behavior.

We already document the default behavior. All having a default will require in addition is adding a very short note to that documentation (as part of adding documentation of the other mode) saying it's the default behavior.