This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch fork-ci-gate-protect-ci-passed in repository https://gitbox.apache.org/repos/asf/airflow.git
commit ce10e70b5fd04c31a92d95c28a2de7d65b84ef9a Author: Jarek Potiuk <[email protected]> AuthorDate: Tue Oct 27 22:49:58 2026 +0100 Close fork CI pull requests that change jobs running in gated runs In a run the gate leaves to fork CI, the gate succeeds and only build-info is skipped, so a job still runs in apache/airflow when its if calls a status check function or all the jobs it needs run. ci-passed is one of them, and it reports the CI passed check the merge ruleset requires. The gate protection only compared which jobs do not depend on the gate, so a gated pull request could change ci-passed, or add a job that needs only the gate, and report CI passed without any CI. GitHub also treats a skipped required check as passed, so renaming any job that is skipped in a gated run to CI passed satisfies the ruleset as well. Job names are therefore part of the gate too. The changed files of a pull request are also listed as it is when listed, so a retarget to a base without the gate while the check ran could close a pull request that should be released to project CI, which is never looked at again once closed. Generated-by: Claude Opus 5 --- .github/workflows/ci-amd.yml | 5 +- .../doc/ci/08_fork_ci_for_external_contributors.md | 23 ++-- .../src/airflow_breeze/utils/fork_ci_reconcile.py | 135 ++++++++++++++++----- dev/breeze/tests/test_fork_ci_reconcile.py | 134 ++++++++++++++++++-- scripts/ci/prek/check_ci_workflows_in_sync.py | 5 +- 5 files changed, 249 insertions(+), 53 deletions(-) diff --git a/.github/workflows/ci-amd.yml b/.github/workflows/ci-amd.yml index 75c3ff8edc4..579c607e08e 100644 --- a/.github/workflows/ci-amd.yml +++ b/.github/workflows/ci-amd.yml @@ -165,8 +165,9 @@ jobs: # Fork CI gate (AIP-120): decides whether project CI runs for this event. Configuration and # code come from the base branch so a pull request cannot change its own eligibility. See # dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. A pull request whose CI runs - # in the contributor's fork and changes this job, or how build-info depends on it, is closed - # by the fork CI monitor ("Gate protection" in that document). + # in the contributor's fork and changes this job, how build-info depends on it, any job that + # runs when it skips build-info (such as ci-passed), or the name of any job is closed by the + # fork CI monitor ("Gate protection" in that document). name: "Fork CI gate" runs-on: ["ubuntu-slim"] timeout-minutes: 5 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 10a96244cfe..b756a6ea216 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 @@ -700,11 +700,17 @@ with a comment explaining why when it changes any of: `[pilot]` entry, which is how contributors join and leave the pilot (the configuration is parsed from the head commit and compared with its version at the pull request's merge base, so comments and later changes on the base branch do not count); -- the `gate` job of `.github/workflows/ci-amd.yml`, the `needs` and `if` that make - `build-info` depend on it, the set of jobs that do not depend on the gate (directly or through - other jobs), or the workflow-level `on`, `defaults`, `env`, `permissions` and `concurrency` (the head and merge-base versions are parsed and those parts - compared, so other changes to the workflow are left to the green-trust check; a head version - that does not parse as a workflow counts as a change). +- the `gate` job of `.github/workflows/ci-amd.yml`, the `needs` and `if` that make `build-info` + depend on it, the whole definition of every job that runs when the gate skips `build-info` + (one whose `if` calls `always()`, `success()`, `failure()` or `cancelled()`, in any case, or + all of whose `needs` run, such as `ci-passed`, which reports the `CI passed` check the merge + ruleset requires), including whether such jobs are added or removed, the `name` of any job, + with its `strategy` when the name is an expression (GitHub treats a skipped required check as + passed, so a job skipped in a gated run and renamed to `CI passed` would satisfy the merge + ruleset), or the workflow-level `on`, `defaults`, `env`, `permissions` and `concurrency` (the + head and merge-base versions are parsed and those parts compared, so other changes to the + workflow are left to the green-trust check; a head version that does not parse as a workflow, + uses YAML aliases, or cannot be compared counts as a change). A changed-file listing at GitHub's 3000-file cap may leave out a gate file, so such a pull request is closed as one whose gate change cannot be checked. @@ -712,8 +718,11 @@ request is closed as one whose gate change cannot be checked. The state comment's marker records `"gate_checked": true` with the head commit only when this check ran for it, so a marker written before the check existed, or while `use fork ci` exempted the pull request, does not count, and a retarget since the state began makes it check again. -The changed files are listed for the pull request as it is when listed, so the head commit is -read again afterwards: if it moved, nothing is recorded and the next run checks the new one. +The changed files are listed for the pull request as it is when listed, so the head commit and +the base branch are read again afterwards: if either changed, nothing is recorded and the next +run checks the pull request again, with the gate configuration of its new base. Otherwise a +retarget to a base without the gate in between could close a pull request that should be +released to project CI. The state is `gate_modified`: the pull request is drafted, loses `ready for maintainer review` and `needs project ci`, gets an `error` status linking its changed files, and the closure is 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 79b5035ac7c..78e86abdff2 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py @@ -30,6 +30,7 @@ from __future__ import annotations import base64 import functools import json +import re from collections.abc import Callable, Iterable, Sequence from dataclasses import asdict, dataclass, field, replace from datetime import datetime, timedelta @@ -1386,15 +1387,19 @@ class BypassFinder: def get_gate_wiring(workflow_text: str | None) -> Any: """ The parts of a ci-amd.yml that make up the gate: its job, how build-info depends on it, the - jobs that do not depend on it (a job freed from the gate runs whatever it decides), and the - workflow-level triggers and settings that every job, the gate included, runs with. + jobs that run when the gate skips build-info, the names of all jobs, and the workflow-level + triggers and settings that every job, the gate included, runs with. A file that is not a workflow mapping yields an object equal to nothing, so a pull request - breaking the workflow counts as changing the gate rather than escaping the check. + breaking the workflow counts as changing the gate rather than escaping the check. So does a + file with YAML aliases, which ci-amd.yml does not use: an alias nested in itself expands + exponentially when compared. """ if workflow_text is None: return None try: + if any(isinstance(event, yaml.AliasEvent) for event in yaml.parse(workflow_text)): + return object() workflow = yaml.safe_load(workflow_text) except yaml.YAMLError: return object() @@ -1407,7 +1412,8 @@ def get_gate_wiring(workflow_text: str | None) -> Any: jobs.get("gate"), build_info.get("needs"), build_info.get("if"), - find_jobs_outside_gate(jobs), + find_jobs_run_when_gated(jobs), + get_job_names(jobs), workflow.get("on", workflow.get(True)), workflow.get("defaults"), workflow.get("env"), @@ -1416,24 +1422,73 @@ def get_gate_wiring(workflow_text: str | None) -> Any: ) -def find_jobs_outside_gate(jobs: dict[str, Any]) -> frozenset[str]: - """Jobs that do not depend on the gate job, directly or through other jobs.""" +# A job whose `if` calls one of these runs although a job it needs was skipped: GitHub prefixes +# `success() &&` only to an `if` without one, so even `success() || true` runs. Function names are +# case-insensitive in GitHub expressions. +STATUS_CHECK_FUNCTION = re.compile(r"\b(?:always|success|failure|cancelled)\s*\(", re.IGNORECASE) - def get_needs(job_id: str) -> list[str]: - job = jobs.get(job_id) - needs = job.get("needs") if isinstance(job, dict) else None - if isinstance(needs, str): - return [needs] - return [need for need in needs if isinstance(need, str)] if isinstance(needs, list) else [] - def depends_on_gate(job_id: str, seen: frozenset[str] = frozenset()) -> bool: - # `seen` stops a dependency cycle, which GitHub rejects anyway. - return any( - need == "gate" or (need not in seen and depends_on_gate(need, seen | {job_id})) - for need in get_needs(job_id) - ) +def find_jobs_run_when_gated(jobs: dict[Any, Any]) -> list[tuple[Any, Any]]: + """ + The definitions of the jobs that run when the gate leaves a pull request to fork CI. + + The gate succeeds then and skips build-info. A job runs if its `if` calls a status check + function, or if every job it needs runs, so also one that only needs a job that runs, such + as `ci-passed`. Such a job reports in apache/airflow whatever it is defined to, `CI passed` + included, so its whole definition is part of the gate. Anything unexpected counts as running. + """ + runs: dict[Any, bool] = {"gate": True, "build-info": False} + + def has_status_check(job: Any) -> bool: + if not isinstance(job, dict): + return True + condition = job.get("if") + if condition is None or isinstance(condition, bool): + return False + return not isinstance(condition, str) or bool(STATUS_CHECK_FUNCTION.search(condition)) + + def get_needs(job: Any) -> list[Any]: + needs = job.get("needs") + needs = needs if isinstance(needs, list) else [needs] if needs is not None else [] + # A need that cannot be a job id stands for a job that is not in the workflow. + return [need if isinstance(need, (str, int, float, bool)) else object() for need in needs] + + def is_run(job_id: Any, seen: frozenset[Any]) -> bool: + if job_id not in runs: + job = jobs.get(job_id) + # A cycle, which GitHub rejects anyway, or a need that is not a job of the workflow. + if job_id in seen or job_id not in jobs: + return True + runs[job_id] = has_status_check(job) or all( + is_run(need, seen | {job_id}) for need in get_needs(job) + ) + return runs[job_id] + + return sorted( + ( + (job_id, job) + for job_id, job in jobs.items() + if job_id not in ("gate", "build-info") and is_run(job_id, frozenset()) + ), + key=lambda entry: repr(entry[0]), + ) + - return frozenset(job_id for job_id in jobs if job_id != "gate" and not depends_on_gate(job_id)) +def get_job_names(jobs: dict[Any, Any]) -> list[tuple[Any, Any, Any]]: + """ + The name of every job, with its matrix when the name may read it. + + GitHub treats a skipped required check as passed, so a job renamed to `CI passed` satisfies + the merge ruleset in a run the gate left to fork CI, where it is skipped. A job without a name + is named after its id, which cannot contain the space that `CI passed` has. + """ + names = [] + for job_id, job in jobs.items(): + name = job.get("name") if isinstance(job, dict) else job + reads_matrix = name is not None and not (isinstance(name, str) and "${{" not in name) + strategy = job.get("strategy") if isinstance(job, dict) and reads_matrix else None + names.append((job_id, name, strategy)) + return sorted(names, key=lambda entry: repr(entry[0])) def is_own_pilot_change(base_text: str | None, head_text: str | None, author_login: str) -> bool: @@ -1456,8 +1511,8 @@ def is_own_pilot_change(base_text: str | None, head_text: str | None, author_log return replace(head, pilot=base.pilot) == base and changed <= {author_login.casefold()} -class HeadMovedError(RuntimeError): - """The pull request's head moved while it was checked; the next run checks the new head.""" +class PullRequestChangedError(RuntimeError): + """The pull request's head or base changed while it was checked; the next run checks it again.""" class GateChangeFinder: @@ -1476,18 +1531,34 @@ class GateChangeFinder: ) return compare["merge_base_commit"]["sha"] + @staticmethod + def _changes_gate_wiring(base_text: str | None, head_text: str | None) -> bool: + # The head version is the contributor's: a workflow that the comparison fails on, such as + # one with a needs chain deeper than the recursion limit, counts as a change. + try: + return get_gate_wiring(base_text) != get_gate_wiring(head_text) + except Exception: + return True + def find(self, pr: PullRequestSnapshot) -> str | None: files = self._client.rest_get( f"repos/{self._repo}/pulls/{pr.number}/files?per_page=100", paginate=True ) # The listing is of the pull request as it is now, while the head commit recorded as checked - # is the one read at the start of the run: a push in between would mark a commit checked - # by another one's files. - current_head = self._client.rest_get(f"repos/{self._repo}/pulls/{pr.number}")["head"]["sha"] + # and the base whose gate configuration classifies the result are those read at the start + # of the run: a push in between would mark a commit checked by another one's files, and a + # retarget would close for a gate change a pull request whose new base has no gate. + current = self._client.rest_get(f"repos/{self._repo}/pulls/{pr.number}") + current_head = current["head"]["sha"] if current_head != pr.head_sha: - raise HeadMovedError( + raise PullRequestChangedError( f"head moved from {pr.head_sha[:8]} to {current_head[:8]} during the gate check" ) + current_base = current["base"]["ref"] + if current_base != pr.base_ref: + raise PullRequestChangedError( + f"base changed from {pr.base_ref} to {current_base} during the gate check" + ) # A listing at GitHub's cap may leave out a gate file, so it is not trusted. if len(files) >= PULL_REQUEST_FILES_CAP: return f"it changes {len(files)} or more files, more than GitHub lists, so the gate cannot be checked" @@ -1504,12 +1575,14 @@ class GateChangeFinder: pr.author_login, ): return f"it changes `{GATE_CONFIG_PATH}` beyond the author's own `[pilot]` entry" - if CI_WORKFLOW_PATH in paths and get_gate_wiring( - read_file(self._client, self._repo, CI_WORKFLOW_PATH, merge_base) - ) != get_gate_wiring(read_file(self._client, head_repo, CI_WORKFLOW_PATH, pr.head_sha)): + if CI_WORKFLOW_PATH in paths and self._changes_gate_wiring( + read_file(self._client, self._repo, CI_WORKFLOW_PATH, merge_base), + read_file(self._client, head_repo, CI_WORKFLOW_PATH, pr.head_sha), + ): return ( - f"it changes the `gate` job of `{CI_WORKFLOW_PATH}`, which jobs depend on it, or the " - "workflow-level triggers or settings it runs with" + f"it changes the `gate` job of `{CI_WORKFLOW_PATH}`, a job that runs when the gate " + "leaves a pull request to fork CI, the name of a job, or the workflow-level triggers or " + "settings they run with" ) return None @@ -1854,7 +1927,7 @@ def run_reconcile( client.delete_run(pr.number, run.id) unknown.extend((pr.number, reason) for reason in plan.unknown) attention.extend((pr.number, item) for item in plan.attention) - except HeadMovedError as e: + except PullRequestChangedError as e: console_print(f"[info]#{listed.number}: {e}; checked again on the next run[/]") except Exception as e: console_print(f"[error]#{listed.number}: {e}[/]") diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index fc4294a1f83..7d906d37023 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -45,9 +45,9 @@ from airflow_breeze.utils.fork_ci_reconcile import ( ForkObservation, ForkState, GateChangeFinder, - HeadMovedError, JobStatusReporter, Plan, + PullRequestChangedError, PullRequestSnapshot, apply_plan, build_job_statuses, @@ -1599,7 +1599,7 @@ def test_run_reconcile_isolates_failures_and_reports_them(client, tmp_path): if path.endswith("/files?per_page=100"): return [] if path.startswith(f"repos/{REPO}/pulls/"): - return {"head": {"sha": f"sha{path.rsplit('/', 1)[1]}"}} + return {"head": {"sha": f"sha{path.rsplit('/', 1)[1]}"}, "base": {"ref": "main"}} raise AssertionError(path) client.rest_get.side_effect = rest_get @@ -1649,7 +1649,7 @@ def test_run_reconcile_rereads_workflow_state_while_awaiting_push(client, tmp_pa if path.endswith("/files?per_page=100"): return [] if path.startswith(f"repos/{REPO}/pulls/"): - return {"head": {"sha": "sha1"}} + return {"head": {"sha": "sha1"}, "base": {"ref": "main"}} raise AssertionError(path) client.rest_get.side_effect = rest_get @@ -2433,18 +2433,26 @@ jobs: needs: [gate] if: needs.gate.outputs.project-ci == 'true' tests: + name: ${{ matrix.suite }} tests + strategy: {matrix: {suite: [core, providers]}} needs: [build-info] steps: [{run: tests}] + ci-passed: + name: ${{ (needs.build-info.result == 'skipped' && 'CI passed (project CI not run)') || 'CI passed' }} + if: always() + needs: [gate, build-info, tests] + steps: [{run: check}] """ GATE_JOB_CHANGE = ( - "it changes the `gate` job of `.github/workflows/ci-amd.yml`, which jobs depend on it, " - "or the workflow-level triggers or settings it runs with" + "it changes the `gate` job of `.github/workflows/ci-amd.yml`, a job that runs when the gate " + "leaves a pull request to fork CI, the name of a job, or the workflow-level triggers or " + "settings they run with" ) -def _gate_change_finder(client, changed, head_files, base_files=None, head_sha="abc123"): +def _gate_change_finder(client, changed, head_files, base_files=None, head_sha="abc123", base_ref="main"): base_files = { ".github/workflows/ci-amd.yml": CI_AMD, ".github/fork-ci-gate.toml": PILOT_BASE, @@ -2457,7 +2465,7 @@ def _gate_change_finder(client, changed, head_files, base_files=None, head_sha=" if "/compare/" in path: return {"merge_base_commit": {"sha": "merge-base"}} if path == f"repos/{REPO}/pulls/42": - return {"head": {"sha": head_sha}} + return {"head": {"sha": head_sha}, "base": {"ref": base_ref}} repo_name, rest = path.removeprefix("repos/").split("/contents/") name, ref = rest.split("?ref=") files = base_files if (repo_name, ref) == (REPO, "merge-base") else head_files @@ -2531,6 +2539,97 @@ def _gate_change_finder(client, changed, head_files, base_files=None, head_sha=" GATE_JOB_CHANGE, id="job-freed-from-the-gate", ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/workflows/ci-amd.yml": CI_AMD.replace( + "'CI passed (project CI not run)'", "'CI passed'" + ) + }, + GATE_JOB_CHANGE, + id="ci-passed-renamed", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/workflows/ci-amd.yml": CI_AMD + + " extra:\n name: CI passed\n needs: [gate]\n steps: [{run: check}]\n" + }, + GATE_JOB_CHANGE, + id="job-run-when-gated-added", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/workflows/ci-amd.yml": CI_AMD.replace( + "needs: [build-info]\n steps: [{run: tests}]", + "needs: [build-info]\n if: always()\n steps: [{run: tests}]", + ) + }, + GATE_JOB_CHANGE, + id="job-made-to-run-when-gated", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/workflows/ci-amd.yml": CI_AMD.replace( + "needs: [build-info]\n steps: [{run: tests}]", + "needs: [build-info]\n if: Success() || true\n steps: [{run: tests}]", + ) + }, + GATE_JOB_CHANGE, + id="job-made-to-run-when-gated-by-explicit-success", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + {".github/workflows/ci-amd.yml": CI_AMD.replace("${{ matrix.suite }} tests", "CI passed")}, + GATE_JOB_CHANGE, + id="skipped-job-renamed", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + {".github/workflows/ci-amd.yml": CI_AMD.replace("[core, providers]", "[core, CI passed]")}, + GATE_JOB_CHANGE, + id="matrix-of-expression-name-changed", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/workflows/ci-amd.yml": CI_AMD + + " after-ci-passed:\n needs: [ci-passed]\n steps: [{run: publish}]\n" + }, + GATE_JOB_CHANGE, + id="job-needing-only-a-job-run-when-gated", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/workflows/ci-amd.yml": CI_AMD.replace( + "steps: [{run: gate}]", "steps: &s [{run: gate}]" + ).replace("steps: [{run: tests}]", "steps: *s") + }, + GATE_JOB_CHANGE, + id="yaml-alias", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/workflows/ci-amd.yml": CI_AMD + + " 1:\n steps: [{run: a}]\n '1':\n steps: [{run: b}]\n" + }, + GATE_JOB_CHANGE, + id="job-ids-alike-as-strings", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/workflows/ci-amd.yml": CI_AMD + + "".join(f" j{i}:\n needs: [j{i + 1}]\n" for i in range(3000)) + + " j3000:\n needs: [build-info]\n" + }, + GATE_JOB_CHANGE, + id="needs-chain-deeper-than-the-recursion-limit", + ), pytest.param( [{"filename": ".github/workflows/ci-amd.yml"}], { @@ -2572,10 +2671,23 @@ def test_marker_records_whether_the_gate_was_checked(pr_overrides, gate_checked) assert ('"gate_checked": true' in plan.marker_comment) is gate_checked -def test_gate_change_finder_aborts_when_the_head_moved(client): - finder = _gate_change_finder(client, [], {}, head_sha="newer") [email protected]( + ("head_sha", "base_ref", "match"), + [ + pytest.param("newer", "main", "head moved from abc123 to newer", id="head-moved"), + pytest.param("abc123", "v3-1-test", "base changed from main to v3-1-test", id="base-changed"), + ], +) +def test_gate_change_finder_aborts_when_the_pull_request_changed(client, head_sha, base_ref, match): + finder = _gate_change_finder( + client, + [{"filename": ".github/workflows/ci-amd.yml"}], + {".github/workflows/ci-amd.yml": CI_AMD.replace("[gate]", "[]")}, + head_sha=head_sha, + base_ref=base_ref, + ) - with pytest.raises(HeadMovedError, match="head moved from abc123 to newer"): + with pytest.raises(PullRequestChangedError, match=match): finder.find(_pr()) @@ -2595,7 +2707,7 @@ def test_run_reconcile_retries_a_pull_request_whose_head_moved(client, tmp_path) if path == f"repos/{REPO}/pulls/1/files?per_page=100": return [] if path == f"repos/{REPO}/pulls/1": - return {"head": {"sha": "moved"}} + return {"head": {"sha": "moved"}, "base": {"ref": "main"}} raise AssertionError(path) client.rest_get.side_effect = rest_get diff --git a/scripts/ci/prek/check_ci_workflows_in_sync.py b/scripts/ci/prek/check_ci_workflows_in_sync.py index 7043131d242..15edf610d7f 100755 --- a/scripts/ci/prek/check_ci_workflows_in_sync.py +++ b/scripts/ci/prek/check_ci_workflows_in_sync.py @@ -162,8 +162,9 @@ AMD_ONLY_GATE_BLOCK = """ gate: # Fork CI gate (AIP-120): decides whether project CI runs for this event. Configuration and # code come from the base branch so a pull request cannot change its own eligibility. See # dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. A pull request whose CI runs - # in the contributor's fork and changes this job, or how build-info depends on it, is closed - # by the fork CI monitor ("Gate protection" in that document). + # in the contributor's fork and changes this job, how build-info depends on it, any job that + # runs when it skips build-info (such as ci-passed), or the name of any job is closed by the + # fork CI monitor ("Gate protection" in that document). name: "Fork CI gate" runs-on: ["ubuntu-slim"] timeout-minutes: 5
