This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch fork-ci-6-reconciler-followups in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 7496b74bf62a7120b5d04125c904587d7fdf2e6d Author: Jarek Potiuk <[email protected]> AuthorDate: Sat Oct 17 02:59:49 2026 +0200 Keep fork CI pull requests sharing a head commit across bases for a maintainer The green-trust decision is stored as a commit status, which every pull request with that head commit shares. Pull requests with different bases have different changed files, so a decision made for one could mark the other ready without checking it. Generated-by: Claude Opus 5 --- .../doc/ci/08_fork_ci_for_external_contributors.md | 5 +++- .../src/airflow_breeze/utils/fork_ci_reconcile.py | 27 ++++++++++++++++++--- dev/breeze/tests/test_fork_ci_reconcile.py | 28 +++++++++++++++++++++- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md index f5482f19b19..0820e007ebf 100644 --- a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md +++ b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md @@ -470,7 +470,10 @@ The decision is read back from the `Fork CI / Tests (AMD)` commit status on the which only `apache/airflow`'s own tokens can write, so it is made once per commit and cannot be forged through a comment. The changed files depend on the base branch as well, so a decision recorded before the pull request was last retargeted (a `BaseRefChangedEvent` in the timeline -newer than the status) is made again, and the status is written again to date it. +newer than the status) is made again, and the status is written again to date it. A commit status +belongs to the commit, so it cannot hold one decision per base: when another gated pull +request with a different base has the same head commit, both are `green_unverified` and wait +for a maintainer, reported to Slack once. This is a guard against the cheapest way to fake a green run, not a guarantee. Any change can weaken the tests it ships with (a `conftest.py` that skips tests, for example), exactly as it diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py index 7e49ed5395b..faef461b4a8 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py @@ -347,6 +347,7 @@ def classify( repo: str, find_bypass_run: Callable[[], str | None], changes_ci_files: Callable[[], bool], + head_shared_across_bases: bool = False, ) -> Plan: """ Decide the mutations for one gated pull request. @@ -357,6 +358,8 @@ def classify( called for pull requests that the gate tampering check applies to. :param changes_ci_files: whether the pull request changes CI-defining paths; only called when a new head commit turns green in the fork. + :param head_shared_across_bases: another gated pull request has the same head commit and a + different base, so their changed files differ but they share one commit status. """ plan = Plan() labels = set(pr.labels) @@ -448,14 +451,22 @@ def classify( # The decision is read back from the commit status on the head commit, which only # apache/airflow's own tokens can write, never from a comment. The changed files depend # on the base too, so a status written before the pull request was retargeted is stale. - # The status is shared by every pull request with this head commit; scoping it to one is - # tracked at https://github.com/apache/airflow/issues/74907 previous = status_state(pr.current_status) decided = previous in (ForkState.GREEN.value, ForkState.GREEN_UNVERIFIED.value) stale = pr.last_base_change_at is not None and ( pr.status_created_at is None or pr.status_created_at <= pr.last_base_change_at ) - if decided and not stale: + if head_shared_across_bases: + # One commit status cannot hold a decision per base, and trusting the one written for + # the other pull request could skip the check; both wait for a maintainer instead. + unverified = True + if previous != ForkState.GREEN_UNVERIFIED.value: + plan.attention.append( + "fork CI is green, but another gated pull request to a different base has the " + "same head commit, so its CI-defining changes cannot be told apart; review and add " + "`use project ci` to verify" + ) + elif decided and not stale: unverified = previous == ForkState.GREEN_UNVERIFIED.value else: # Writing the status again, even unchanged, records when this decision was made. @@ -1014,6 +1025,14 @@ def pull_request_changes_ci_files(client: GitHubClient, repo: str, number: int) return changes_ci_definition(changed_paths(files), file_count=len(files)) +def find_heads_shared_across_bases(prs: Iterable[PullRequestSnapshot]) -> set[str]: + """Head commits of more than one pull request whose bases differ.""" + bases_by_head: dict[str, set[str]] = {} + for pr in prs: + bases_by_head.setdefault(pr.head_sha, set()).add(pr.base_ref) + return {head for head, bases in bases_by_head.items() if len(bases) > 1} + + def apply_plan(client: GitHubClient, pr: PullRequestSnapshot, plan: Plan) -> None: if plan.draft is True: client.convert_to_draft(pr.number, pr.node_id) @@ -1129,6 +1148,7 @@ def run_reconcile( client, repo=repo, now=now, base_triggers=load_workflow_triggers(workflows_dir) ) + shared_heads = find_heads_shared_across_bases(prs) for listed in prs: try: pr = listed if listed.comments_cursor is None else find_state_comment(client, repo, listed) @@ -1149,6 +1169,7 @@ def run_reconcile( repo=repo, find_bypass_run=functools.partial(bypass_finder.find, pr), changes_ci_files=functools.partial(pull_request_changes_ci_files, client, repo, pr.number), + head_shared_across_bases=pr.head_sha in shared_heads, ) apply_plan(client, pr, plan) unknown.extend((pr.number, reason) for reason in plan.unknown) diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index 059da404cac..e5b28b13572 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -42,6 +42,7 @@ from airflow_breeze.utils.fork_ci_reconcile import ( discover, fetch_fork_observations, fetch_workflow_state, + find_heads_shared_across_bases, find_state_comment, jobs_show_project_ci, load_workflow_triggers, @@ -83,7 +84,9 @@ def _marker(state: str, *, days_ago: float = 0, sha: str = "abc123") -> Marker: return Marker(state=state, sha=sha, since=NOW - timedelta(days=days_ago)) -def _classify(pr, fork=None, *, bypass_run=None, changes_ci_files=False, config=CONFIG): +def _classify( + pr, fork=None, *, bypass_run=None, changes_ci_files=False, config=CONFIG, head_shared_across_bases=False +): find_bypass = mock.Mock(return_value=bypass_run) changes = mock.Mock(return_value=changes_ci_files) plan = classify( @@ -94,6 +97,7 @@ def _classify(pr, fork=None, *, bypass_run=None, changes_ci_files=False, config= repo=REPO, find_bypass_run=find_bypass, changes_ci_files=changes, + head_shared_across_bases=head_shared_across_bases, ) return plan, find_bypass, changes @@ -314,6 +318,28 @@ def test_green_trust_is_decided_again_after_retargeting(status_created_at, lists assert (plan.status == GREEN_STATUS) is reposts_status +def test_green_is_not_trusted_for_a_head_shared_across_bases(): + plan, _, changes = _classify( + _pr(current_status=GREEN_STATUS), GREEN, changes_ci_files=False, head_shared_across_bases=True + ) + + assert plan.state == ForkState.GREEN_UNVERIFIED + assert plan.add_labels == ["needs project ci"] + assert len(plan.attention) == 1 + changes.assert_not_called() + + +def test_find_heads_shared_across_bases(): + prs = [ + _pr(number=1, head_sha="a", base_ref="main"), + _pr(number=2, head_sha="a", base_ref="v3-1-test"), + _pr(number=3, head_sha="b", base_ref="main"), + _pr(number=4, head_sha="b", base_ref="main"), + ] + + assert find_heads_shared_across_bases(prs) == {"a"} + + @pytest.mark.parametrize( ("files", "expected"), [
