diff --git a/.github/scripts/release_attribution.py b/.github/scripts/release_attribution.py index 3eb96d6da6..d069f52f47 100644 --- a/.github/scripts/release_attribution.py +++ b/.github/scripts/release_attribution.py @@ -519,6 +519,27 @@ def unresolved_coauthor_identities( return sorted(unresolved.values(), key=lambda item: (item["email"], item["sha"])) +def is_clean_main_sync(github, repository, sha, parents, main_sha) -> bool: + if len(parents) != 2 or not main_sha: + return False + try: + ancestry = github.get(f"repos/{repository}/compare/{parents[1]}...{main_sha}") + if ancestry.get("status") not in {"ahead", "identical"}: + return False + changed_paths = [] + for parent in parents: + files = github.get(f"repos/{repository}/compare/{parent}...{sha}").get("files") + if files is None or len(files) >= 300: + return False + changed_paths.append( + {file.get(key) for file in files for key in ("filename", "previous_filename")} + - {None} + ) + return changed_paths[0].isdisjoint(changed_paths[1]) + except (ReleaseError, OSError, ValueError, KeyError, TypeError): + return False + + def validate_pr_attribution( *, repository: str, @@ -527,6 +548,7 @@ def validate_pr_attribution( base_config: Mapping[str, Any], identity_changes: Mapping[str, str | None], github: GitHubClient, + main_sha: str = "", ) -> None: """Fail closed when preserved human authorship cannot reach a GitHub login. @@ -624,9 +646,11 @@ def record_unresolved( # # A base synchronization merge imports a parent outside the PR commit set, # so it is not salvaged contributor work. Its distinct author must still - # have explicit coauthor credit elsewhere in the PR. + # have explicit coauthor credit elsewhere in the PR unless it is a clean main sync. if is_base_sync_merge and distinct_external_author: - if author_login.lower() not in credited_coauthors: + if author_login.lower() not in credited_coauthors and not is_clean_main_sync( + github, repository, sha, parents, main_sha + ): trailer = f"Co-authored-by: {author_name} <{author_email}>" early_errors.append( f"base synchronization merge {sha} is authored by @{author_login}, " @@ -1467,6 +1491,7 @@ def validate_pr_command(args: argparse.Namespace) -> None: base_config=base_config, identity_changes=identity_changes, github=client, + main_sha=trusted_sha, ) print(f"contributor attribution is merge-ready for pull request #{number}") diff --git a/.github/scripts/tests/test_pr_attribution.py b/.github/scripts/tests/test_pr_attribution.py index c78c6a6185..d3d6d7fc92 100644 --- a/.github/scripts/tests/test_pr_attribution.py +++ b/.github/scripts/tests/test_pr_attribution.py @@ -58,9 +58,16 @@ def commit( class SourceGitHub: - def __init__(self, authors, source_emails): + def __init__(self, authors, source_emails, comparisons=None): self.authors = authors self.source_emails = source_emails + self.comparisons = comparisons or {} + + def get(self, path): + value = self.comparisons[path] + if isinstance(value, Exception): + raise value + return value def pull(self, repository: str, number: int): assert repository == "trycua/cua" @@ -81,6 +88,8 @@ def validate( changes=None, authors=None, source_emails=None, + comparisons=None, + main_sha="", ): base = base or config() validate_pr_attribution( @@ -89,7 +98,8 @@ def validate( commits=commits or [], base_config=base, identity_changes=changes or {}, - github=SourceGitHub(authors or {}, source_emails or {}), + github=SourceGitHub(authors or {}, source_emails or {}, comparisons), + main_sha=main_sha, ) @@ -456,3 +466,68 @@ def test_compare_pages_that_stop_short_are_refused(): client = PagedCompareGitHub(total=250, reported_total=300) with pytest.raises(release_attribution.ReleaseError, match="refusing partial"): client.all_pull_commits("trycua/cua-staging", large_pull()) + + +@pytest.mark.parametrize( + "status, branch_files, main_files, failure, clean", + [ + ("ahead", ["main-change"], ["branch-change"], False, True), + ("identical", ["main-change"], ["branch-change"], False, True), + ("ahead", ["conflict"], ["conflict"], False, False), + ("ahead", [{"filename": "q", "previous_filename": "p"}], ["p"], False, False), + ("diverged", ["main-change"], ["branch-change"], False, False), + ("ahead", [str(i) for i in range(300)], ["branch-change"], False, False), + ("ahead", ["main-change"], [str(i) for i in range(300)], False, False), + ("ahead", ["main-change"], ["branch-change"], True, False), + (None, ["main-change"], ["branch-change"], False, False), + ("ahead", ["main-change"], ["branch-change"], "missing-files", False), + ], +) +def test_main_sync_merge_authorship(status, branch_files, main_files, failure, clean): + comparisons = { + "repos/trycua/cua/compare/main-tip...trusted-main": {"status": status} if status else {}, + "repos/trycua/cua/compare/feature-tip...sync-merge": { + "files": [ + name if isinstance(name, dict) else {"filename": name} for name in branch_files + ] + }, + "repos/trycua/cua/compare/main-tip...sync-merge": ( + ReleaseError("API failed") + if failure is True + else ( + {} + if failure == "missing-files" + else {"files": [{"filename": name} for name in main_files]} + ) + ), + } + values = dict( + commits=[ + commit(sha="feature-tip", login="landing-author"), + commit( + sha="sync-merge", + login="injaneity", + email="44902825+injaneity@users.noreply.github.com", + parent_shas=("feature-tip", "main-tip"), + ), + ], + comparisons=comparisons, + main_sha="trusted-main", + ) + if clean: + validate(**values) + unlinked = commit( + sha="sync-merge", + email="unlinked@institution.example", + parent_shas=("feature-tip", "main-tip"), + ) + with pytest.raises(ReleaseError, match="unlinked commit author"): + validate(**{**values, "commits": [values["commits"][0], unlinked]}) + values["commits"][1]["commit"][ + "message" + ] += "\n\nCo-authored-by: Pair " + with pytest.raises(ReleaseError, match="has no explicit same-repository source PR"): + validate(**values) + else: + with pytest.raises(ReleaseError, match="base synchronization merge"): + validate(**values)