From 0df295a0ff5c5d302770f589497280132131c63d Mon Sep 17 00:00:00 2001 From: Takashi Kokubun Date: Tue, 31 Mar 2026 15:34:47 -0700 Subject: [PATCH] Fix last_files_match_only to evaluate per file instead of globally Previously, when last_files_match_only was enabled, every time a pattern matched any changed file, all previously accumulated reviewers were cleared. This meant the last pattern that matched ANY file in the PR determined all reviewers, which was wrong when different files had different last-matching patterns. Now, for each changed file independently, we find its last matching pattern's reviewers, then union the results across all files. This correctly handles the case where e.g. a generated file overrides reviewers to [] while a non-generated file in the same directory still matches the wildcard pattern's reviewers. --- dist/index.js | 28 ++++++++++++++++++++-------- src/reviewer.js | 30 +++++++++++++++++++++--------- test/reviewer.test.js | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 17 deletions(-) diff --git a/dist/index.js b/dist/index.js index b80ea99..fbea931 100644 --- a/dist/index.js +++ b/dist/index.js @@ -16215,16 +16215,28 @@ function identify_reviewers_by_changed_files({ config, changed_files, excludes = return []; } - const matching_reviewers = []; + let matching_reviewers; - Object.entries(config.files).forEach(([ glob_pattern, reviewers ]) => { - if (changed_files.some((changed_file) => minimatch(changed_file, glob_pattern))) { - if (last_files_match_only) { - matching_reviewers.length = 0; // clear previous matches + if (last_files_match_only) { + const file_patterns = Object.entries(config.files); + const per_file_reviewers = changed_files.flatMap((changed_file) => { + let last_matching_reviewers = null; + file_patterns.forEach(([ glob_pattern, reviewers ]) => { + if (minimatch(changed_file, glob_pattern)) { + last_matching_reviewers = reviewers; + } + }); + return last_matching_reviewers || []; + }); + matching_reviewers = per_file_reviewers; + } else { + matching_reviewers = []; + Object.entries(config.files).forEach(([ glob_pattern, reviewers ]) => { + if (changed_files.some((changed_file) => minimatch(changed_file, glob_pattern))) { + matching_reviewers.push(...reviewers); } - matching_reviewers.push(...reviewers); - } - }); + }); + } const individuals = replace_groups_with_individuals({ reviewers: matching_reviewers, config }); diff --git a/src/reviewer.js b/src/reviewer.js index 086ef66..bd40fe5 100644 --- a/src/reviewer.js +++ b/src/reviewer.js @@ -48,16 +48,28 @@ function identify_reviewers_by_changed_files({ config, changed_files, excludes = return []; } - const matching_reviewers = []; - - Object.entries(config.files).forEach(([ glob_pattern, reviewers ]) => { - if (changed_files.some((changed_file) => minimatch(changed_file, glob_pattern))) { - if (last_files_match_only) { - matching_reviewers.length = 0; // clear previous matches + let matching_reviewers; + + if (last_files_match_only) { + const file_patterns = Object.entries(config.files); + const per_file_reviewers = changed_files.flatMap((changed_file) => { + let last_matching_reviewers = null; + file_patterns.forEach(([ glob_pattern, reviewers ]) => { + if (minimatch(changed_file, glob_pattern)) { + last_matching_reviewers = reviewers; + } + }); + return last_matching_reviewers || []; + }); + matching_reviewers = per_file_reviewers; + } else { + matching_reviewers = []; + Object.entries(config.files).forEach(([ glob_pattern, reviewers ]) => { + if (changed_files.some((changed_file) => minimatch(changed_file, glob_pattern))) { + matching_reviewers.push(...reviewers); } - matching_reviewers.push(...reviewers); - } - }); + }); + } const individuals = replace_groups_with_individuals({ reviewers: matching_reviewers, config }); diff --git a/test/reviewer.test.js b/test/reviewer.test.js index 83facc1..25e96de 100644 --- a/test/reviewer.test.js +++ b/test/reviewer.test.js @@ -126,6 +126,43 @@ describe('reviewer', function() { }; expect(identify_reviewers_by_changed_files({ config: config_with_last_files_match_only, changed_files })).to.have.members([ 'mario', 'someone-specific' ]); }); + + context('with `last_files_match_only` and a specific file override with empty reviewers', function() { + const override_config = { + reviewers: { + groups: { + 'team-a': [ 'alice', 'bob' ], + }, + }, + files: { + 'src/**/*': [ 'team-a' ], + 'src/generated-file.dat': [], + }, + options: { + last_files_match_only: true, + }, + }; + + it('returns reviewers when a file only matches the wildcard pattern', function() { + const changed_files = [ 'src/main-code.js' ]; + expect(identify_reviewers_by_changed_files({ config: override_config, changed_files })).to.have.members([ 'alice', 'bob' ]); + }); + + it('returns no reviewers when a file only matches the overriding empty pattern', function() { + const changed_files = [ 'src/generated-file.dat' ]; + expect(identify_reviewers_by_changed_files({ config: override_config, changed_files })).to.deep.equal([]); + }); + + it('returns no reviewers when all files are unmatched or overridden with empty reviewers', function() { + const changed_files = [ 'unrelated-file.txt', 'src/generated-file.dat' ]; + expect(identify_reviewers_by_changed_files({ config: override_config, changed_files })).to.deep.equal([]); + }); + + it('returns reviewers when one file matches the wildcard and another is overridden with empty reviewers', function() { + const changed_files = [ 'src/main-code.js', 'src/generated-file.dat' ]; + expect(identify_reviewers_by_changed_files({ config: override_config, changed_files })).to.have.members([ 'alice', 'bob' ]); + }); + }); }); describe('identify_reviewers_by_author()', function() {