feat(shorebird_ci): reserve required name, verify aggregator coverage (#3785)

This commit is contained in:
nickshorebird
2026-05-19 13:47:46 -04:00
committed by GitHub
parent 63d965e6e4
commit 96bde1b5f7
7 changed files with 594 additions and 26 deletions
+7
View File
@@ -1,5 +1,12 @@
<!-- cspell:words toplevel -->
# 0.2.4
- `verify` now enforces the `--required` aggregator's `needs:` list. Name-based detection: if a workflow has a top-level job named `required`, every other top-level job must appear in its `needs:`, and every entry in `needs:` must match a real top-level job in the same file. A `required:` key w/ no map body is also reported as malformed. Closes three silent-failure modes: a hand-edited workflow could leave a job out of `required.needs` and have its status silently ignored by the merge gate, a typo'd `needs:` entry could go unnoticed until GHA rejected it at runtime, or a bodiless `required:` could pass verify while doing nothing at runtime.
- `generate --required` reserves the `required` job name and refuses to generate when a package slug would collide. Prevents a duplicate YAML key from silently overwriting the aggregator job. Only fires for `--style static` (dynamic mode keys jobs by `setup`/`dart_ci`/`flutter_ci`/`cspell`, so a package named `required` can't collide there). Skip the flag and any package slug is fine.
- Bad CLI args now print a usage message and exit 64 instead of dumping a Dart stack trace. Same as the convention used by other args-based tools.
- `verify` command description and `--required` flag help text updated to cover the new aggregator-consistency checks and the reserved-name contract. `--required` help also rewraps cleanly at the col-30 boundary.
# 0.2.3
- New `--required` flag on `generate`. When set, the generated workflow gets an aggregator `required` job that depends on every other job and uses `if: ${{ always() }}` so it runs even when sub-jobs are skipped. The job fails only if any dependency reports `failure` or `cancelled`, so it's safe to use as the single required check in branch protection: per-package jobs that get skipped because no paths-filter output matched are treated as a pass. Wired into both `--style static` and `--style dynamic`. Skipped by default, so existing generated workflows are unaffected.
+10 -1
View File
@@ -1,7 +1,16 @@
// cspell:words sysexits
import 'dart:io';
import 'package:args/command_runner.dart';
import 'package:shorebird_ci/src/shorebird_ci_command_runner.dart';
Future<void> main(List<String> args) async {
exit(await ShorebirdCiCommandRunner().run(args) ?? 0);
try {
exit(await ShorebirdCiCommandRunner().run(args) ?? 0);
} on UsageException catch (e) {
// Bad CLI args. Print the message + usage and exit 64 (EX_USAGE
// per sysexits.h) instead of dumping a Dart stack trace.
stderr.writeln(e);
exit(64);
}
}
@@ -40,11 +40,11 @@ class GenerateCommand extends Command<int> with RepoRootOption {
)
..addFlag(
'no-update-actions',
help:
'Skip the post-generate step that queries GitHub for current '
'action versions and bumps pins in place. By default, generate '
'auto-bumps action versions to current latest. Pass this flag '
'in offline environments.',
help: '''
Skip the post-generate step that queries GitHub for current action
versions and bumps pins in place. By default, generate auto-bumps
action versions to current latest. Pass this flag in offline
environments.''',
negatable: false,
)
..addOption(
@@ -61,13 +61,15 @@ for whether your repo requires a token to upload.''',
)
..addFlag(
'required',
help:
'Emit a `required` aggregator job that depends on every other '
'job in the workflow. Use it as the single required check in '
'branch protection: it fails if any sub-job failed or was '
'cancelled, and passes when sub-jobs succeed or were skipped '
'(skips are expected since per-package jobs only run on '
'touched paths).',
help: '''
Emit a `required` aggregator job at the end of the workflow that
depends on every other job. Use it as the single required check in
branch protection: the aggregator fails when any dependency reports
`failure` or `cancelled`, and passes when dependencies succeed or
were skipped. Skipped sub-jobs are the common case since per-package
jobs only run on touched paths. The `required` job key is reserved
when this flag is set, so generation fails if any package slug
resolves to `required`. Skip the flag and any package slug is fine.''',
negatable: false,
);
}
@@ -99,6 +101,44 @@ for whether your repo requires a token to upload.''',
return 1;
}
final sortedPackages = repository.packages.toList()
..sort(
(PackageDescription a, PackageDescription b) =>
a.name.compareTo(b.name),
);
// Slugs only matter for static-mode workflow generation: in dynamic
// mode jobs are keyed `setup` / `dart_ci` / `flutter_ci` / `cspell`,
// never by per-package slug, so a package named `required` cannot
// collide w/ the aggregator there. Compute (and collision-check)
// only when static.
Map<PackageDescription, String>? slugs;
if (style == 'static') {
slugs = computePackageSlugs(
packages: sortedPackages,
repoRoot: repoRoot,
);
if (emitRequiredJob) {
// `required` is the reserved job key for the --required
// aggregator. A package slug that resolves to `required` would
// emit a duplicate YAML key and silently overwrite the
// aggregator. Fail loudly so the user renames the package
// before generation.
final colliding = slugs.entries
.where((e) => e.value == 'required')
.map((e) => e.key.name)
.toList();
if (colliding.isNotEmpty) {
stderr.writeln(
'Package slug `required` collides with the --required '
'aggregator job. Rename the colliding package(s): '
'${colliding.join(', ')}',
);
return 1;
}
}
}
// Each builder returns a map of repo-relative path → file content.
// Dynamic returns a single entry; static returns a main workflow
// plus one or two reusable workflows.
@@ -111,6 +151,8 @@ for whether your repo requires a token to upload.''',
outputPath: outputPath,
codecovTokenSecret: codecovTokenSecret,
emitRequiredJob: emitRequiredJob,
packages: sortedPackages,
slugs: slugs!,
)
: {
outputPath: _buildDynamicYaml(
@@ -212,13 +254,9 @@ updates:
required String outputPath,
required String? codecovTokenSecret,
required bool emitRequiredJob,
required List<PackageDescription> packages,
required Map<PackageDescription, String> slugs,
}) {
final packages = repository.packages.toList()
..sort(
(PackageDescription a, PackageDescription b) =>
a.name.compareTo(b.name),
);
final hasDart = packages.any(
(pkg) => !RepositoryAnalyzer.dependsOnFlutter(root: pkg.root),
);
@@ -232,6 +270,7 @@ updates:
packages: packages,
codecovTokenSecret: codecovTokenSecret,
emitRequiredJob: emitRequiredJob,
slugs: slugs,
),
};
if (hasDart) {
@@ -256,16 +295,13 @@ updates:
required List<PackageDescription> packages,
required String? codecovTokenSecret,
required bool emitRequiredJob,
required Map<PackageDescription, String> slugs,
}) {
final emitSecretsInherit =
codecovTokenSecret != null &&
codecovTokenSecret.isNotEmpty &&
repository.hasCodecov;
final resolver = DependencyResolver(repository.root.path);
final slugs = computePackageSlugs(
packages: packages,
repoRoot: repository.root.path,
);
final buffer = StringBuffer()
..write('''
@@ -8,6 +8,7 @@ import 'package:shorebird_ci/src/dorny_filter.dart';
import 'package:shorebird_ci/src/package_description.dart';
import 'package:shorebird_ci/src/package_slug.dart';
import 'package:shorebird_ci/src/repository_analyzer.dart';
import 'package:yaml/yaml.dart';
/// Marker comment that the `generate` command writes into dynamic
/// workflows. Verify uses this to detect dynamic coverage instead of
@@ -16,7 +17,8 @@ import 'package:shorebird_ci/src/repository_analyzer.dart';
const dynamicCoverageMarker = '# shorebird_ci-managed: dynamic';
/// Verifies that every discovered package has CI coverage somewhere in
/// `.github/workflows/`.
/// `.github/workflows/`, and that any `required` aggregator job stays
/// in sync with the rest of the workflow.
///
/// Coverage can be provided in two ways:
/// - **Dynamic**: a workflow that calls `shorebird_ci affected_packages`
@@ -27,6 +29,12 @@ const dynamicCoverageMarker = '# shorebird_ci-managed: dynamic';
/// slug is just the package name; when two packages share a name the
/// slug is `<parent_dir>_<name>`. Missing packages are reported with
/// the dorny entry that should be added (including transitive deps).
///
/// In addition, if any workflow file has a top-level job keyed
/// `required`, every other top-level job in that file must appear in
/// its `needs:`, and every entry in `needs:` must match a real
/// top-level job. The aggregator is the single check listed in branch
/// protection, so drift in either direction silently breaks the gate.
class VerifyCommand extends Command<int> with RepoRootOption {
/// Creates a [VerifyCommand].
VerifyCommand() {
@@ -41,7 +49,8 @@ class VerifyCommand extends Command<int> with RepoRootOption {
String get name => 'verify';
@override
String get description => 'Verify every package has CI coverage';
String get description =>
'Verify package CI coverage and `required` aggregator consistency';
@override
Future<int> run() async {
@@ -93,11 +102,21 @@ class VerifyCommand extends Command<int> with RepoRootOption {
final allPackages = repository.packages.toList()
..sort((a, b) => a.name.compareTo(b.name));
// Required-job consistency check. Name-based: if a workflow has a
// top-level job keyed `required`, every other top-level job must
// appear in its `needs:` list. Runs unconditionally for every
// workflow file, independent of static-vs-dynamic coverage style.
final requiredJobErrors = _findRequiredJobErrors(workflowFiles);
if (dynamicWorkflows.isNotEmpty) {
stdout.writeln(
'Using dynamic coverage via ${dynamicWorkflows.join(', ')}'
'all ${allPackages.length} packages covered at runtime.',
);
if (requiredJobErrors.isNotEmpty) {
_printRequiredJobErrors(requiredJobErrors);
return 1;
}
return 0;
}
@@ -121,6 +140,10 @@ class VerifyCommand extends Command<int> with RepoRootOption {
if (missing.isEmpty) {
stdout.writeln('\nAll packages have CI coverage.');
if (requiredJobErrors.isNotEmpty) {
_printRequiredJobErrors(requiredJobErrors);
return 1;
}
return 0;
}
@@ -147,9 +170,124 @@ class VerifyCommand extends Command<int> with RepoRootOption {
stderr.writeln(
'${missing.length} package(s) missing from CI coverage.',
);
if (requiredJobErrors.isNotEmpty) {
_printRequiredJobErrors(requiredJobErrors);
}
return 1;
}
/// For each workflow file w/ a top-level `required:` job, returns
/// the symmetric drift between its `needs:` list and the set of
/// other top-level jobs in the same file.
///
/// `missing` are top-level jobs absent from `needs:` — they run but
/// their status is silently ignored by the aggregator and branch
/// protection, the exact failure mode this check guards against.
///
/// `stale` are entries in `needs:` w/ no matching top-level job,
/// usually a typo. GHA itself rejects these at runtime, but catching
/// them at verify time keeps the feedback loop tight.
///
/// Workflows without a `required:` job are absent from the result.
Map<String, _RequiredJobReport> _findRequiredJobErrors(
List<File> workflowFiles,
) {
final errors = <String, _RequiredJobReport>{};
for (final file in workflowFiles) {
final fileName = p.basename(file.path);
final YamlMap doc;
try {
final loaded = loadYaml(file.readAsStringSync());
if (loaded is! YamlMap) continue;
doc = loaded;
} on YamlException {
// Skip files we can't parse — verify isn't a YAML linter.
continue;
}
final jobs = doc['jobs'];
if (jobs is! YamlMap) continue;
if (!jobs.containsKey('required')) continue;
final requiredJob = jobs['required'];
if (requiredJob is! YamlMap) {
// `required:` exists as a key but has no map body (e.g. `null`,
// a scalar string, or a list). That's a malformed aggregator —
// GHA wouldn't run it, and verify can't reason about its
// `needs:`. Surface it instead of silently skipping.
errors[fileName] = _RequiredJobReport(
missing: const [],
stale: const [],
isMalformed: true,
);
continue;
}
// `needs:` may be a scalar (single dependency), a list, missing
// entirely, or null. Normalize to a set of strings; unrecognized
// types fall through to an empty set, which surfaces every other
// job as missing — the correct conservative outcome.
final rawNeeds = requiredJob['needs'];
final needsList = <String>[
if (rawNeeds is String) rawNeeds,
if (rawNeeds is YamlList)
for (final n in rawNeeds) n.toString(),
];
final needsSet = needsList.toSet();
final jobNames = <String>{
for (final key in jobs.keys) key.toString(),
};
final missing = <String>[
for (final job in jobNames)
if (job != 'required' && !needsSet.contains(job)) job,
];
final stale = <String>[
for (final need in needsList)
if (!jobNames.contains(need)) need,
];
if (missing.isNotEmpty || stale.isNotEmpty) {
errors[fileName] = _RequiredJobReport(
missing: missing,
stale: stale,
);
}
}
return errors;
}
void _printRequiredJobErrors(Map<String, _RequiredJobReport> errors) {
stderr.writeln();
for (final entry in errors.entries) {
final report = entry.value;
if (report.isMalformed) {
stderr.writeln(
'MALFORMED `required:` job in ${entry.key}: value is not a '
'map. Expected a job definition w/ `needs:` and `runs-on:`.',
);
continue;
}
if (report.missing.isNotEmpty) {
stderr
..writeln('MISSING from required.needs in ${entry.key}:')
..writeln(' ${report.missing.join(', ')}');
}
if (report.stale.isNotEmpty) {
stderr
..writeln('STALE entries in required.needs in ${entry.key}:')
..writeln(' ${report.stale.join(', ')}')
..writeln(' (these reference jobs that do not exist in the file)');
}
}
stderr.writeln(
'\n`required` job must depend on every other job in the workflow, '
'and every entry in `needs:` must match a real job. '
'Re-run `shorebird_ci generate --required` to regenerate.',
);
}
/// Whether a workflow uses the dynamic affected_packages approach.
///
/// Looks for the marker comment that the `generate` command writes.
@@ -165,3 +303,25 @@ class VerifyCommand extends Command<int> with RepoRootOption {
return workflowContent.contains(dynamicCoverageMarker);
}
}
/// Per-workflow drift report for the `required:` aggregator.
class _RequiredJobReport {
_RequiredJobReport({
required this.missing,
required this.stale,
this.isMalformed = false,
});
/// Top-level jobs that exist in the workflow but are absent from
/// `required.needs`. They run but don't gate the aggregator.
final List<String> missing;
/// Entries listed in `required.needs` that don't match any top-level
/// job in the workflow. GHA itself rejects these at runtime.
final List<String> stale;
/// The `required:` key exists but its value isn't a job map. When
/// true, `missing` and `stale` are empty — there's nothing structured
/// to compare.
final bool isMalformed;
}
+1 -1
View File
@@ -3,7 +3,7 @@ description: >-
CI tooling for Dart and Flutter monorepos. Generates GitHub Actions
workflows, resolves affected packages via dependency graphs, and
verifies path filters stay in sync.
version: 0.2.3
version: 0.2.4
homepage: https://shorebird.dev
repository: https://github.com/shorebirdtech/shorebird/tree/main/packages/shorebird_ci
topics: [ci, github-actions, monorepo, shorebird]
@@ -926,6 +926,55 @@ void main() {
final yaml = _readMain(tempDir);
expect(() => loadYaml(yaml), returnsNormally);
});
test('--required + package named `required` → fails at generate', () async {
// `required` is the reserved aggregator job key. A package slug
// that resolves to `required` would duplicate the YAML key and
// silently overwrite the aggregator. Generate must refuse.
createPackage(tempDir, 'packages/foo', 'foo');
createPackage(tempDir, 'packages/required', 'required');
initGitRepo(tempDir);
final exitCode = await runGenerate(
tempDir,
extra: ['--style', 'static', '--required'],
);
expect(exitCode, 1);
});
test(
'--required + package named `required` is fine in dynamic mode',
() async {
// Dynamic mode keys jobs by `setup` / `dart_ci` / `flutter_ci`
// / `cspell`, never by per-package slug, so a package named
// `required` cannot collide w/ the aggregator. The collision
// check only fires for --style static.
createPackage(tempDir, 'packages/required', 'required');
initGitRepo(tempDir);
final exitCode = await runGenerate(
tempDir,
extra: ['--required'],
);
expect(exitCode, 0);
},
);
test('collision check does not fire when --required is absent', () async {
// The reserved-name contract only applies when the user opts into
// the aggregator. A package named `required` is fine on its own.
createPackage(tempDir, 'packages/required', 'required');
initGitRepo(tempDir);
final exitCode = await runGenerate(
tempDir,
extra: ['--style', 'static'],
);
expect(exitCode, 0);
});
});
}
@@ -183,4 +183,311 @@ jobs:
// With --ignore, e2e is skipped.
expect(await runVerify(tempDir, extra: ['--ignore', 'e2e']), 0);
});
group('required job consistency', () {
test('workflow without a required job → no extra check', () async {
createPackage(tempDir, 'packages/foo', 'foo');
_writeWorkflow(tempDir, 'ci.yaml', '''
name: CI
on: [push]
jobs:
changes:
runs-on: ubuntu-latest
steps:
- uses: dorny/paths-filter@v3
with:
filters: |
foo:
- packages/foo/**
foo:
runs-on: ubuntu-latest
steps:
- run: echo
''');
initGitRepo(tempDir);
expect(await runVerify(tempDir), 0);
});
test('required.needs covers every other job → passes', () async {
createPackage(tempDir, 'packages/foo', 'foo');
_writeWorkflow(tempDir, 'ci.yaml', '''
name: CI
on: [push]
jobs:
changes:
runs-on: ubuntu-latest
steps:
- uses: dorny/paths-filter@v3
with:
filters: |
foo:
- packages/foo/**
foo:
runs-on: ubuntu-latest
steps:
- run: echo
required:
needs:
- changes
- foo
runs-on: ubuntu-latest
steps:
- run: echo
''');
initGitRepo(tempDir);
expect(await runVerify(tempDir), 0);
});
test('required.needs missing a job → returns 1', () async {
createPackage(tempDir, 'packages/foo', 'foo');
_writeWorkflow(tempDir, 'ci.yaml', '''
name: CI
on: [push]
jobs:
changes:
runs-on: ubuntu-latest
steps:
- uses: dorny/paths-filter@v3
with:
filters: |
foo:
- packages/foo/**
foo:
runs-on: ubuntu-latest
steps:
- run: echo
required:
needs:
- changes
runs-on: ubuntu-latest
steps:
- run: echo
''');
initGitRepo(tempDir);
expect(await runVerify(tempDir), 1);
});
test('required.needs as scalar string → handled', () async {
createPackage(tempDir, 'packages/foo', 'foo');
_writeWorkflow(tempDir, 'ci.yaml', '''
name: CI
on: [push]
jobs:
changes:
runs-on: ubuntu-latest
steps:
- uses: dorny/paths-filter@v3
with:
filters: |
foo:
- packages/foo/**
foo:
runs-on: ubuntu-latest
steps:
- run: echo
required:
needs: changes
runs-on: ubuntu-latest
steps:
- run: echo
''');
initGitRepo(tempDir);
// `needs: changes` is the YAML scalar form. `foo` is missing.
expect(await runVerify(tempDir), 1);
});
test('required job w/ no needs key → returns 1', () async {
// Extreme drift: aggregator job declared but `needs:` missing
// entirely. Every other job becomes "missing" by definition.
createPackage(tempDir, 'packages/foo', 'foo');
_writeWorkflow(tempDir, 'ci.yaml', '''
name: CI
on: [push]
jobs:
changes:
runs-on: ubuntu-latest
steps:
- uses: dorny/paths-filter@v3
with:
filters: |
foo:
- packages/foo/**
foo:
runs-on: ubuntu-latest
steps:
- run: echo
required:
runs-on: ubuntu-latest
steps:
- run: echo
''');
initGitRepo(tempDir);
expect(await runVerify(tempDir), 1);
});
test('required.needs references a non-existent job → returns 1', () async {
// Typo case: `needs:` lists a job that doesn't exist in the file.
// GHA itself catches this at runtime, but verify catches it earlier.
createPackage(tempDir, 'packages/foo', 'foo');
_writeWorkflow(tempDir, 'ci.yaml', '''
name: CI
on: [push]
jobs:
changes:
runs-on: ubuntu-latest
steps:
- uses: dorny/paths-filter@v3
with:
filters: |
foo:
- packages/foo/**
foo:
runs-on: ubuntu-latest
steps:
- run: echo
required:
needs:
- changes
- foo
- foo_typo
runs-on: ubuntu-latest
steps:
- run: echo
''');
initGitRepo(tempDir);
expect(await runVerify(tempDir), 1);
});
test('required job check fires alongside dynamic coverage', () async {
createPackage(tempDir, 'packages/foo', 'foo');
// Dynamic-coverage workflow (so the package check is satisfied)
// that also has a malformed required job missing the cspell entry.
_writeWorkflow(tempDir, 'ci.yaml', '''
# shorebird_ci-managed: dynamic
name: CI
on: [push]
jobs:
setup:
runs-on: ubuntu-latest
steps:
- run: shorebird_ci affected_packages
cspell:
runs-on: ubuntu-latest
steps:
- run: echo
required:
needs:
- setup
runs-on: ubuntu-latest
steps:
- run: echo
''');
initGitRepo(tempDir);
// Dynamic coverage would pass on its own, but required.needs is
// missing cspell, so verify still fails.
expect(await runVerify(tempDir), 1);
});
test('drift fires alongside missing-package coverage', () async {
// Static-style workflow w/ a package missing from filters AND a
// required.needs that's missing a job. Exercises the
// requiredJobErrors branch on the missing-packages path, so the
// user sees both failures in one run.
createPackage(tempDir, 'packages/foo', 'foo');
createPackage(tempDir, 'packages/bar', 'bar');
_writeWorkflow(tempDir, 'ci.yaml', '''
name: CI
on: [push]
jobs:
changes:
runs-on: ubuntu-latest
steps:
- uses: dorny/paths-filter@v3
with:
filters: |
foo:
- packages/foo/**
foo:
runs-on: ubuntu-latest
steps:
- run: echo
required:
needs:
- changes
runs-on: ubuntu-latest
steps:
- run: echo
''');
initGitRepo(tempDir);
expect(await runVerify(tempDir), 1);
});
test('invalid workflow yaml is skipped, not crashing', () async {
// verify is not a YAML linter. A file we can't parse should
// silently fall out of the required-job check rather than
// throwing.
createPackage(tempDir, 'packages/foo', 'foo');
_writeWorkflow(tempDir, 'bad.yaml', '''
name: CI
on: [push]
jobs:
foo:
runs-on: ubuntu-latest
steps:
- run: |
this: is: not: valid: yaml: [
''');
_writeWorkflow(tempDir, 'ci.yaml', '''
# shorebird_ci-managed: dynamic
name: CI
on: [push]
jobs:
setup:
runs-on: ubuntu-latest
steps:
- run: shorebird_ci affected_packages
''');
initGitRepo(tempDir);
// Dynamic coverage covers the package; the bad file just gets
// skipped during the required-job scan. Verify returns 0.
expect(await runVerify(tempDir), 0);
});
test('malformed `required:` (no map body) → returns 1', () async {
// `required:` exists as a key but has no job-map body. GHA
// wouldn't run it, so verify can't reason about its `needs:`.
// Treat as a hard error rather than silently skipping.
createPackage(tempDir, 'packages/foo', 'foo');
_writeWorkflow(tempDir, 'ci.yaml', '''
name: CI
on: [push]
jobs:
changes:
runs-on: ubuntu-latest
steps:
- uses: dorny/paths-filter@v3
with:
filters: |
foo:
- packages/foo/**
foo:
runs-on: ubuntu-latest
steps:
- run: echo
required:
''');
initGitRepo(tempDir);
expect(await runVerify(tempDir), 1);
});
});
}