From 903efc186439c1a8bc0e3c5070d42f49ef70b2d2 Mon Sep 17 00:00:00 2001 From: John McDole Date: Sun, 13 Sep 2026 09:40:40 -0700 Subject: [PATCH] refactor(ci): extract process runner and harden workflow git error handling - Extract ProcessRunner and ProcessLogger utilities for subprocess execution. - Support throwOnError in defaultGitList so validators fail visibly on git error. - Propagate git discovery failures in RfcValidator as actionable errors. - Set fetch-depth: 0 in workflows to ensure base branch refs are available. - Remove error silencing in rfc-lint workflow to fail fast on invalid refs. --- .github/workflows/rfc-lint.yml | 19 +++-- .github/workflows/test.yml | 2 +- .github/workflows/validate-rfc-number.yml | 2 +- lib/logprocess.dart | 5 ++ lib/src/assigner.dart | 1 + lib/src/git_lister.dart | 76 +++++++++---------- lib/src/process_logger.dart | 36 +++++++++ lib/src/process_runner.dart | 9 +++ lib/src/validator.dart | 41 ++++++++++- test/git_lister_test.dart | 89 +++++++++++++++++------ test/mock_process_runner.dart | 7 +- test/validate_rfc_number_test.dart | 54 +++++++++++++- 12 files changed, 264 insertions(+), 77 deletions(-) create mode 100644 lib/logprocess.dart create mode 100644 lib/src/process_logger.dart create mode 100644 lib/src/process_runner.dart diff --git a/.github/workflows/rfc-lint.yml b/.github/workflows/rfc-lint.yml index e4faf37..2333f36 100644 --- a/.github/workflows/rfc-lint.yml +++ b/.github/workflows/rfc-lint.yml @@ -23,7 +23,7 @@ jobs: - name: Checkout Code uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - fetch-depth: ${{ github.event_name == 'pull_request' && 0 || 1 }} + fetch-depth: 0 persist-credentials: false - name: Setup Dart @@ -38,14 +38,17 @@ jobs: env: BASE_REF: ${{ github.base_ref }} run: | - BASE_TARGET="origin/${BASE_REF:-main}" - if ! git rev-parse --verify "$BASE_TARGET" >/dev/null 2>&1; then - BASE_TARGET="main" - fi - FILES=$(git diff --name-only --diff-filter=ACMR "$BASE_TARGET"...HEAD -- 'rfc/*.md' 2>/dev/null || true) - if [ -z "$FILES" ]; then - FILES=$(git diff --name-only --diff-filter=ACMR "$BASE_TARGET" -- 'rfc/*.md' 2>/dev/null || true) + TARGET_REF="${BASE_REF:-main}" + TARGET_REF="${TARGET_REF#origin/}" + if git rev-parse --verify "origin/$TARGET_REF" >/dev/null 2>&1; then + BASE_TARGET="origin/$TARGET_REF" + elif git rev-parse --verify "$TARGET_REF" >/dev/null 2>&1; then + BASE_TARGET="$TARGET_REF" + else + echo "::error::Cannot resolve base branch ref 'origin/$TARGET_REF' or '$TARGET_REF'." + exit 1 fi + FILES=$(git diff --name-only --diff-filter=ACMR "$BASE_TARGET"...HEAD -- 'rfc/*.md') if [ -n "$FILES" ]; then echo "has_changes=true" >> "$GITHUB_OUTPUT" { diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2195988..23adf86 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -27,7 +27,7 @@ jobs: - name: Checkout Code uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - fetch-depth: ${{ github.event_name == 'pull_request' && 0 || 1 }} + fetch-depth: 0 persist-credentials: false - name: Setup Dart diff --git a/.github/workflows/validate-rfc-number.yml b/.github/workflows/validate-rfc-number.yml index b372970..b161aa3 100644 --- a/.github/workflows/validate-rfc-number.yml +++ b/.github/workflows/validate-rfc-number.yml @@ -22,7 +22,7 @@ jobs: - name: Checkout Code uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - fetch-depth: ${{ github.event_name == 'pull_request' && 0 || 1 }} + fetch-depth: 0 persist-credentials: false - name: Setup Dart diff --git a/lib/logprocess.dart b/lib/logprocess.dart new file mode 100644 index 0000000..0c89e3c --- /dev/null +++ b/lib/logprocess.dart @@ -0,0 +1,5 @@ +// Copyright 2026 The Flutter Authors. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +export 'src/process_logger.dart'; diff --git a/lib/src/assigner.dart b/lib/src/assigner.dart index db52cb0..3fd1440 100644 --- a/lib/src/assigner.dart +++ b/lib/src/assigner.dart @@ -10,6 +10,7 @@ import 'package:path/path.dart' as p; import 'git_lister.dart'; import 'git_lister.dart' as git_lister; import 'models/rfc_file.dart'; +import 'process_runner.dart'; export 'git_lister.dart' show GitListFunction, defaultGitList; diff --git a/lib/src/git_lister.dart b/lib/src/git_lister.dart index d6eb124..6390aee 100644 --- a/lib/src/git_lister.dart +++ b/lib/src/git_lister.dart @@ -3,11 +3,10 @@ // found in the LICENSE file. import 'dart:convert'; -import 'dart:io' show Process, ProcessResult, stdout, stderr; +import 'dart:io' show Process, ProcessException, ProcessResult; -/// Signature for running an external process asynchronously. -typedef ProcessRunner = - Future Function(String executable, List arguments); +import 'process_logger.dart'; +import 'process_runner.dart'; /// Function signature for discovering RFC filenames in the main branch via git. typedef GitListFunction = Future> Function({String baseBranch}); @@ -21,52 +20,55 @@ Set parseLsTreeOutput(dynamic stdout) { }; } -void _logGitError(ProcessResult result) { - stderr.writeln('exit code: ${result.exitCode}'); - stdout.writeln('git ls-tree stdout:'); - stdout.writeln(result.stdout); - stderr.writeln('git ls-tree stderr:'); - stderr.writeln(result.stderr); -} - /// Discovers RFC filenames in main branch via git. Future> defaultGitList({ String baseBranch = 'origin/main', ProcessRunner processRunner = Process.run, + bool throwOnError = false, }) async { - try { - final result = await processRunner('git', [ - 'ls-tree', - '-r', - '--name-only', - baseBranch, - 'rfc/', - ]); - if (result.exitCode == 0) { - return parseLsTreeOutput(result.stdout); - } + final trimmedBranch = baseBranch.trim(); + final cleanBranch = trimmedBranch.replaceFirst( + RegExp(r'^(?:remotes\/)?(?:origin|upstream)\/'), + '', + ); + final branchesToTry = [ + trimmedBranch, + if (cleanBranch != trimmedBranch) cleanBranch, + ]; - final cleanBranch = baseBranch.replaceFirst( - RegExp(r'^(?:remotes\/)?(?:origin|upstream)\/'), - '', - ); - if (cleanBranch != baseBranch) { - final locResult = await processRunner('git', [ + ProcessResult? lastResult; + String? lastBranch; + for (final branch in branchesToTry) { + lastBranch = branch; + try { + final result = await processRunner('git', [ 'ls-tree', '-r', '--name-only', - cleanBranch, + branch, 'rfc/', ]); - if (locResult.exitCode == 0) { - return parseLsTreeOutput(locResult.stdout); + if (result.exitCode == 0) { + return parseLsTreeOutput(result.stdout); } - _logGitError(locResult); - } else { - _logGitError(result); + lastResult = result; + logProcessResult(result, command: 'git ls-tree'); + } catch (e) { + logProcessError(e, command: 'git ls-tree'); + if (throwOnError) rethrow; } - } catch (e) { - stderr.writeln('git ls-tree exception: $e'); + } + + if (throwOnError && lastResult != null) { + final err = '${lastResult.stderr}'.trim(); + throw ProcessException( + 'git', + ['ls-tree', '-r', '--name-only', lastBranch ?? trimmedBranch, 'rfc/'], + err.isNotEmpty + ? err + : 'git ls-tree failed with exit code ${lastResult.exitCode}', + lastResult.exitCode, + ); } return {}; } diff --git a/lib/src/process_logger.dart b/lib/src/process_logger.dart new file mode 100644 index 0000000..ecbf08f --- /dev/null +++ b/lib/src/process_logger.dart @@ -0,0 +1,36 @@ +// Copyright 2026 The Flutter Authors. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' show ProcessResult, stderr, stdout; + +/// Logs the details of an external [ProcessResult] (exit code, stdout, stderr). +/// +/// When [onLog] or [onError] are provided, log lines are dispatched to them; +/// otherwise, exit code and stderr output are sent to [stderr], and stdout +/// output is sent to [stdout]. +void logProcessResult( + ProcessResult result, { + String command = 'process', + void Function(String message)? onLog, + void Function(String message)? onError, +}) { + final out = onLog ?? stdout.writeln; + final err = onError ?? stderr.writeln; + + err('exit code: ${result.exitCode}'); + out('$command stdout:'); + out('${result.stdout}'); + err('$command stderr:'); + err('${result.stderr}'); +} + +/// Logs an exception encountered during process execution. +void logProcessError( + Object error, { + String command = 'process', + void Function(String message)? onError, +}) { + final err = onError ?? stderr.writeln; + err('$command exception: $error'); +} diff --git a/lib/src/process_runner.dart b/lib/src/process_runner.dart new file mode 100644 index 0000000..7d01bdf --- /dev/null +++ b/lib/src/process_runner.dart @@ -0,0 +1,9 @@ +// Copyright 2026 The Flutter Authors. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' show ProcessResult; + +/// Signature for running an external process asynchronously. +typedef ProcessRunner = + Future Function(String executable, List arguments); diff --git a/lib/src/validator.dart b/lib/src/validator.dart index 9611d02..9670d4b 100644 --- a/lib/src/validator.dart +++ b/lib/src/validator.dart @@ -2,13 +2,16 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:io' show Process, ProcessException; import 'dart:math'; import 'package:file/file.dart'; import 'package:path/path.dart' as p; import 'git_lister.dart'; +import 'git_lister.dart' as git_lister; import 'github_annotation.dart'; import 'models/rfc_file.dart'; +import 'process_runner.dart'; export 'git_lister.dart' show GitListFunction, defaultGitList; @@ -64,7 +67,20 @@ class RfcValidator { final FileSystem fs; final GitListFunction gitList; - const RfcValidator({required this.fs, this.gitList = defaultGitList}); + const RfcValidator({ + required this.fs, + this.gitList = RfcValidator.defaultGitList, + }); + + /// Discovers RFC filenames in base branch via git, throwing on failure. + static Future> defaultGitList({ + String baseBranch = 'origin/main', + ProcessRunner processRunner = Process.run, + }) => git_lister.defaultGitList( + baseBranch: baseBranch, + processRunner: processRunner, + throwOnError: true, + ); /// Runs semantic validation on all RFC files in the repository. /// @@ -131,9 +147,26 @@ class RfcValidator { } // Discover and index base branch RFCs if --check-base is requested. - final baseBranchCategories = { - if (checkBase) ...await _indexBaseBranchBySubsystem(baseBranch), - }; + Map baseBranchCategories = const {}; + if (checkBase) { + try { + baseBranchCategories = await _indexBaseBranchBySubsystem(baseBranch); + } catch (e) { + final message = switch (e) { + ProcessException(:final message, :final errorCode) => + message.isNotEmpty ? message : 'Exit code $errorCode', + _ => '$e', + }; + errors.add( + ValidationError( + filePath: '', + message: + "Failed to discover RFCs on base branch '$baseBranch': $message", + ), + ); + return (isSuccess: false, errors: errors); + } + } // For each category AAA: // - verify no duplicates diff --git a/test/git_lister_test.dart b/test/git_lister_test.dart index 8c06de2..38aec9f 100644 --- a/test/git_lister_test.dart +++ b/test/git_lister_test.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:io' show ProcessException; + import 'package:rfc_tools/src/git_lister.dart'; import 'package:test/test.dart'; @@ -31,31 +33,76 @@ void main() { ); }); - test('returns empty set on non-zero exit code', () async { - final runner = MockProcessRunner( - exitCode: 128, - stderr: 'fatal: not a valid object name', - ); + test( + 'returns empty set on non-zero exit code when throwOnError is false', + () async { + final runner = MockProcessRunner( + exitCode: 128, + stderr: 'fatal: not a valid object name', + ); - final files = await defaultGitList( - baseBranch: 'origin/main', - processRunner: runner.run, - ); + final files = await defaultGitList( + baseBranch: 'origin/main', + processRunner: runner.run, + throwOnError: false, + ); - expect(files, isEmpty); - }); + expect(files, isEmpty); + }, + ); - test('returns empty set when process throws', () async { - final runner = MockProcessRunner( - exceptionToThrow: Exception('process failed'), - ); + test( + 'throws ProcessException on non-zero exit code when throwOnError is true', + () async { + final runner = MockProcessRunner( + exitCode: 128, + stderr: 'fatal: not a valid object name', + ); - final files = await defaultGitList( - baseBranch: 'origin/main', - processRunner: runner.run, - ); + expect( + () => defaultGitList( + baseBranch: 'origin/main', + processRunner: runner.run, + throwOnError: true, + ), + throwsA(isA()), + ); + }, + ); - expect(files, isEmpty); - }); + test( + 'returns empty set when processRunner throws and throwOnError is false', + () async { + final runner = MockProcessRunner( + exceptionToThrow: Exception('process failed'), + ); + + final files = await defaultGitList( + baseBranch: 'origin/main', + processRunner: runner.run, + throwOnError: false, + ); + + expect(files, isEmpty); + }, + ); + + test( + 'rethrows when processRunner throws and throwOnError is true', + () async { + final runner = MockProcessRunner( + exceptionToThrow: Exception('process failed'), + ); + + expect( + () => defaultGitList( + baseBranch: 'origin/main', + processRunner: runner.run, + throwOnError: true, + ), + throwsA(isA()), + ); + }, + ); }); } diff --git a/test/mock_process_runner.dart b/test/mock_process_runner.dart index 132b52d..c2ba492 100644 --- a/test/mock_process_runner.dart +++ b/test/mock_process_runner.dart @@ -4,14 +4,15 @@ import 'dart:io'; -typedef MockProcessHandler = - Future Function(String executable, List arguments); +import 'package:rfc_tools/src/process_runner.dart'; + +export 'package:rfc_tools/src/process_runner.dart' show ProcessRunner; /// In-memory mock process runner to record external process invocations and /// control process outputs hermetically in tests. class MockProcessRunner { final List<({String executable, List arguments})> calls = []; - MockProcessHandler? handler; + ProcessRunner? handler; int exitCode; dynamic stdout; dynamic stderr; diff --git a/test/validate_rfc_number_test.dart b/test/validate_rfc_number_test.dart index 4754e0c..80efee5 100644 --- a/test/validate_rfc_number_test.dart +++ b/test/validate_rfc_number_test.dart @@ -2,10 +2,14 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:io' show ProcessException; + import 'package:file/memory.dart'; import 'package:rfc_tools/src/validator.dart'; import 'package:test/test.dart'; +import 'mock_process_runner.dart'; + void main() { group('RfcValidator', () { late MemoryFileSystem fs; @@ -206,11 +210,29 @@ title: Feature expect(gitListCalled, isFalse); }); - test('default constructor uses defaultGitList', () { + test('default constructor uses RfcValidator.defaultGitList', () { final validator = RfcValidator(fs: fs); - expect(validator.gitList, equals(defaultGitList)); + expect(validator.gitList, equals(RfcValidator.defaultGitList)); }); + test( + 'RfcValidator.defaultGitList throws ProcessException on non-zero exit code', + () async { + final runner = MockProcessRunner( + exitCode: 128, + stderr: 'fatal: not a valid object name', + ); + + expect( + () => RfcValidator.defaultGitList( + baseBranch: 'origin/main', + processRunner: runner.run, + ), + throwsA(isA()), + ); + }, + ); + group('sequential numbering gap checks', () { test('rejects gap when new RFC jumps ahead of baseBranch', () async { await fs.file('rfc/110.0050-jump-ahead.md').writeAsString('''--- @@ -453,6 +475,34 @@ title: Draft expect(result.isValid, isTrue); expect(result.errors, isEmpty); }); + + test('fails with error when gitList throws', () async { + await fs.file('rfc/110.0001-feature.md').writeAsString('''--- +type: rfc +rfc: '110.0001' +title: Feature +--- +'''); + + final validator = RfcValidator( + fs: fs, + gitList: ({String baseBranch = 'origin/main'}) async => + throw ProcessException( + 'git', + ['ls-tree'], + 'fatal: not a valid object name', + 128, + ), + ); + + final result = await validator.validate(checkBase: true); + expect(result.isValid, isFalse); + expect(result.errors, hasLength(1)); + expect( + result.errors.first.message, + contains('Failed to discover RFCs on base branch'), + ); + }); }); group('ValidationError', () {