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 5ffe91d7c8744837341479b286f7726c3fc3f1a7 Author: Jarek Potiuk <[email protected]> AuthorDate: Sat Oct 17 03:23:01 2026 +0200 Close fork CI pull requests that change the fork CI gate A pull request whose CI runs in the contributor's fork could change the gate that sent it there: its code, its configuration, the gate job or the monitor. Such a change decides where the pull request's own CI runs, so the monitor now closes it and explains why. Contributors can still add, change or remove their own pilot entry, which is how they join and leave the pilot. Generated-by: Claude Opus 5 --- .github/fork-ci-gate.toml | 5 + .github/workflows/ci-amd.yml | 4 +- .github/workflows/fork-ci-monitor.yml | 3 + contributing-docs/25_maintainer_pr_triage.md | 6 +- .../doc/ci/08_fork_ci_for_external_contributors.md | 41 +++ .../src/airflow_breeze/utils/fork_ci_gate.py | 5 + .../src/airflow_breeze/utils/fork_ci_github.py | 8 +- .../src/airflow_breeze/utils/fork_ci_messages.py | 38 ++- .../src/airflow_breeze/utils/fork_ci_reconcile.py | 240 ++++++++++++++-- dev/breeze/tests/test_fork_ci_messages.py | 7 + dev/breeze/tests/test_fork_ci_reconcile.py | 313 ++++++++++++++++++++- scripts/ci/prek/check_ci_workflows_in_sync.py | 4 +- 12 files changed, 646 insertions(+), 28 deletions(-) diff --git a/.github/fork-ci-gate.toml b/.github/fork-ci-gate.toml index c80593d221c..688f91da9fb 100644 --- a/.github/fork-ci-gate.toml +++ b/.github/fork-ci-gate.toml @@ -16,6 +16,11 @@ # under the License. # Fork CI gate (AIP-120). See dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md +# +# This file is part of the gate. A pull request whose CI runs in the contributor's fork may only +# add, change or remove its author's own [pilot] entry here; the fork CI monitor closes it if it +# changes anything else, because that change would decide where its own CI runs. See +# "Gate protection" in the document above. # Master switch. When false the gate job lets every pull request run project CI, except # pull requests labelled `use fork ci` and pull requests of [pilot] participants. Push diff --git a/.github/workflows/ci-amd.yml b/.github/workflows/ci-amd.yml index d85aaddeffe..32a52525217 100644 --- a/.github/workflows/ci-amd.yml +++ b/.github/workflows/ci-amd.yml @@ -71,7 +71,9 @@ jobs: 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. + # 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). name: "Fork CI gate" runs-on: ["ubuntu-slim"] timeout-minutes: 5 diff --git a/.github/workflows/fork-ci-monitor.yml b/.github/workflows/fork-ci-monitor.yml index d963b9235e4..2e0e2463857 100644 --- a/.github/workflows/fork-ci-monitor.yml +++ b/.github/workflows/fork-ci-monitor.yml @@ -20,6 +20,9 @@ # state comment, the `Fork CI / Tests (AMD)` commit status and closing. It is the only writer for # those pull requests and never checks out or runs pull request code. See # dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. +# +# Part of the fork CI gate: a pull request whose CI runs in the contributor's fork and changes +# this file is closed by this monitor ("Gate protection" in the document above). name: "Fork CI monitor" on: # yamllint disable-line rule:truthy schedule: diff --git a/contributing-docs/25_maintainer_pr_triage.md b/contributing-docs/25_maintainer_pr_triage.md index 2196f8db177..3dff86be287 100644 --- a/contributing-docs/25_maintainer_pr_triage.md +++ b/contributing-docs/25_maintainer_pr_triage.md @@ -249,7 +249,7 @@ The triage workflow uses the following labels and states: ## Pull requests gated to fork CI Pull requests labelled `fork ci required` run their CI in the contributor's fork. The scheduled -fork CI reconciler (`breeze ci fork-ci-reconcile`, run by `fork-ci-monitor.yml`) owns four things +fork CI reconciler (`breeze ci fork-ci-reconcile`, run by `fork-ci-monitor.yml`) owns five things for them, and the triage skill must treat those pull requests as out of scope for each: - **Draft state.** The reconciler keeps the pull request in draft until the fork run for its @@ -259,6 +259,10 @@ for them, and the triage skill must treat those pull requests as out of scope fo - **`needs project ci`.** The reconciler adds it when the fork run is green but the pull request changes CI-defining files, and removes it once the state is anything else. These pull requests stay in draft and wait for a maintainer to review the CI change and add `use project ci`. +- **Closing for changing the gate.** The reconciler closes a pull request that changes the fork + CI gate (its code, configuration other than the author's own `[pilot]` entry, the `gate` job + of `ci-amd.yml` or the monitor), and reports it on Slack. Add `use project ci` to accept such + a change. - **Closing after inactivity.** The reconciler closes a pull request whose fork CI has not run for `close_after_days`. 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 d5703324a12..559e2b236c0 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 @@ -410,6 +410,7 @@ pull requests in that state, not by the total. | **green_unverified**: conclusion success, but the pull request changes CI-defining files (see "Trusting a green fork run") | draft | remove | `pending` (yellow), link to the run | update: a maintainer must run project CI; reported to Slack once per commit | | setup_required or awaiting_push for `close_after_days` | unchanged | unchanged | unchanged | close with a new comment; reopening restarts the clock | | **ungated**: the base branch has no gate, for example after retargeting to an older release branch | undraft\* | unchanged | `success` (green), link to the pull request's checks | update: project CI is in use; `fork ci required` is removed | +| **gate_modified**: the pull request changes the fork CI gate (see "Gate protection") | draft | remove | `error` (red), link to the pull request's changed files | post a **new** comment explaining why, then close; checked after gate tampering, before the fork state | | **gate_bypassed**: a recorded project CI run of the pull request's branch bypassed the gate (see "Gate tampering") | draft | remove | `error` (red), link to the project CI run that should not have happened | post a **new** comment explaining why, then close; takes precedence over every row above | \* only if the last convert-to-draft event on the pull request was by `github-actions[bot]`; @@ -562,6 +563,46 @@ signal 1 no longer holds and the pull request re-enters the fork state machine. ci` releases it like any other gated pull request; a maintainer who wants to keep an external workflow change has to leave that label on. +### Gate protection + +The gate decides where a pull request's CI runs, so a pull request whose CI runs in a fork must +not change it: the change would decide where its own CI runs, and a maintainer reviewing it later +would be reviewing a pull request that already escaped the gate. For every pull request the +tampering check applies to (`external` and `pilot`), the reconciler lists the changed files once +per head commit, counting a renamed file under both paths, and closes the pull request +with a comment explaining why when it changes any of: + +- `dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py`, the reconciler modules + `fork_ci_reconcile.py`, `fork_ci_github.py` and `fork_ci_messages.py`, or + `.github/workflows/fork-ci-monitor.yml`; +- `.github/fork-ci-gate.toml`, other than adding, changing or removing the author's own + `[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). + +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. + +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 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 +reported to Slack as an attention item. Gate tampering is checked first and wins. As for gate +tampering, only a reopen by someone other than the author counts: an open pull request whose +marker says `gate_modified` and that was not reopened since is closed again, and a maintainer's +reopen without restoring the gate closes it again on the next run. A maintainer who wants the +change adds `use project ci`, which releases the pull request before any of these checks. Each +of these files carries a comment saying so. + ### State comment One comment per pull request, created on first contact and edited afterwards. It starts with diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py index 23770bdc76c..8074f50ac71 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py @@ -22,6 +22,11 @@ to CI in the contributor's fork. See dev/breeze/doc/ci/08_fork_ci_for_external_c The gate job in ``ci-amd.yml`` runs this file with the runner's ``python3`` from a sparse checkout, without installing breeze, so it must only import from the standard library. + +This file is part of the fork CI gate. A pull request whose CI runs in the contributor's fork +must not change it, because the change would decide where its own CI runs: the fork CI +monitor closes such a pull request with a comment explaining why. See "Gate protection" in +the document above. """ from __future__ import annotations diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_github.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_github.py index 7d73e224c17..ecc471a0237 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_github.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_github.py @@ -14,7 +14,13 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -"""GitHub access for the fork CI reconciler (AIP-120); the only module of it that calls ``gh``.""" +""" +GitHub access for the fork CI reconciler (AIP-120); the only module of it that calls ``gh``. + +Part of the fork CI gate: a pull request whose CI runs in the contributor's fork and changes +this file is closed by the fork CI monitor. See "Gate protection" in +dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. +""" from __future__ import annotations diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py index ea42f0ded6b..61498708bfe 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py @@ -20,6 +20,10 @@ Texts the fork CI reconciler posts on pull requests (AIP-120). All wording lives here so it can be reviewed in one place. Texts must say that CI is free for the contributor and finite for the project, must not characterise the recipient, and must give the exact command to run. + +Part of the fork CI gate: a pull request whose CI runs in the contributor's fork and changes +this file is closed by the fork CI monitor. See "Gate protection" in +dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. """ from __future__ import annotations @@ -63,12 +67,16 @@ class Marker: state: str sha: str since: datetime + # Set only by a reconciler that ran the gate protection check for this head commit, so a + # marker written before that check existed, or while it did not apply, never counts as one. + gate_checked: bool = False def render_marker(marker: Marker) -> str: - payload = json.dumps( - {"state": marker.state, "sha": marker.sha, "since": marker.since.isoformat()}, sort_keys=True - ) + fields: dict[str, object] = {"state": marker.state, "sha": marker.sha, "since": marker.since.isoformat()} + if marker.gate_checked: + fields["gate_checked"] = True + payload = json.dumps(fields, sort_keys=True) return f"{MARKER_PREFIX}{payload} -->" @@ -79,7 +87,12 @@ def parse_marker(body: str) -> Marker | None: return None try: data = json.loads(match.group("payload")) - return Marker(state=data["state"], sha=data["sha"], since=datetime.fromisoformat(data["since"])) + return Marker( + state=data["state"], + sha=data["sha"], + since=datetime.fromisoformat(data["since"]), + gate_checked=data.get("gate_checked") is True, + ) except (json.JSONDecodeError, KeyError, TypeError, ValueError) as e: raise ValueError(f"Malformed fork CI marker: {match.group(0)!r}") from e @@ -119,6 +132,7 @@ _STATE_TEXTS = { "comment below for how to continue.", "gate_bypassed": "**Fork CI: closed.** Project CI ran for this pull request although its CI runs in " "the contributor's fork; see the comment below.", + "gate_modified": "**Fork CI: closed.** This pull request changes the fork CI gate; see the comment below.", } @@ -140,6 +154,20 @@ def released_comment(marker: Marker) -> str: ) +def gate_modified_close_comment(*, change: str | None) -> str: + return ( + f"Closing this pull request because it changes the fork CI gate{f': {change}' if change else ''}.\n\n" + "The gate decides whether a pull request runs its CI in apache/airflow or in the contributor's " + "fork. It is made of the `gate` job of `.github/workflows/ci-amd.yml`, " + "`dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py`, `.github/fork-ci-gate.toml` and the fork CI " + "monitor, and a pull request whose CI runs in a fork must not change it, because the change would " + "decide where its own CI runs. The one exception is adding, changing or removing your own `[pilot]` " + "entry in `.github/fork-ci-gate.toml`.\n\n" + "If the change is wanted, a maintainer can reopen the pull request and add the `use project ci` " + "label." + FOOTER + ) + + def ungated_comment(marker: Marker, *, base_ref: str) -> str: return ( f"{render_marker(marker)}\n\n" @@ -184,6 +212,7 @@ _STATUS_DESCRIPTIONS = { "gate_bypassed": "Project CI ran for a gated pull request - see Details", "released": "Project CI in use (label `use project ci`) - see the checks", "ungated": "Project CI in use (base branch without fork CI) - see the checks", + "gate_modified": "Pull request changes the fork CI gate - see the comment", } STATUS_STATES = { @@ -196,6 +225,7 @@ STATUS_STATES = { "gate_bypassed": "error", "released": "success", "ungated": "success", + "gate_modified": "error", } 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 81f71b19cce..4c6ffcd9843 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py @@ -20,6 +20,9 @@ Scheduled reconciler for pull requests gated to fork CI (AIP-120). ``classify`` is a pure function from what GitHub recorded about one pull request to the mutations that bring it to the desired state; ``run_reconcile`` gathers that data and applies the result. See "Reconciler" in dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md. + +Part of the fork CI gate: a pull request whose CI runs in the contributor's fork and changes +this file is closed by the fork CI monitor, see "Gate protection" in the same document. """ from __future__ import annotations @@ -57,6 +60,7 @@ from airflow_breeze.utils.fork_ci_messages import ( STATUS_STATES, Marker, fork_actions_url, + gate_modified_close_comment, inactivity_close_comment, nudge_comment, parse_marker, @@ -71,6 +75,19 @@ CI_WORKFLOW_FILE = "ci-amd.yml" # The files the gate job of ci-amd.yml reads from a pull request's base branch. GATE_CONFIG_PATH = ".github/fork-ci-gate.toml" GATE_SCRIPT_PATH = "dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py" +CI_WORKFLOW_PATH = f".github/workflows/{CI_WORKFLOW_FILE}" +# Files of the fork CI gate that a pull request whose CI runs in a fork must not change at all. +# The gate configuration and the gate job in ci-amd.yml are checked separately, since the first +# may carry the author's own pilot entry and the second shares its file with the CI pipeline. +GATE_PROTECTED_PATHS = frozenset( + { + GATE_SCRIPT_PATH, + "dev/breeze/src/airflow_breeze/utils/fork_ci_github.py", + "dev/breeze/src/airflow_breeze/utils/fork_ci_messages.py", + "dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py", + ".github/workflows/fork-ci-monitor.yml", + } +) GATE_JOB_NAME = "Fork CI gate" BOT_LOGINS = frozenset({"github-actions", "github-actions[bot]"}) DISCOVERY_WINDOW = timedelta(hours=24) @@ -120,6 +137,7 @@ class ForkState(str, Enum): CLOSED = "closed" RELEASED = "released" UNGATED = "ungated" + GATE_MODIFIED = "gate_modified" CLOSABLE_STATES = frozenset({ForkState.SETUP_REQUIRED, ForkState.AWAITING_PUSH}) @@ -350,6 +368,7 @@ def classify( find_bypass_run: Callable[[], str | None], changes_ci_files: Callable[[], bool], head_shared_across_bases: bool = False, + find_gate_change: Callable[[], str | None] = lambda: None, ) -> Plan: """ Decide the mutations for one gated pull request. @@ -362,6 +381,9 @@ def classify( 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. + :param find_gate_change: describes how the pull request changes the fork CI gate, or returns + None; only called for pull requests that the gate tampering check applies to, once per + head commit. """ plan = Plan() labels = set(pr.labels) @@ -424,6 +446,7 @@ def classify( reopened_since_closing = { ForkState.CLOSED.value: pr.last_reopened_by_anyone_at, ForkState.GATE_BYPASSED.value: pr.last_reopened_at, + ForkState.GATE_MODIFIED.value: pr.last_reopened_at, } if marker and marker.state in reopened_since_closing: reopened = reopened_since_closing[marker.state] @@ -432,6 +455,8 @@ def classify( _set_label(plan, labels, LABEL_NEEDS_PROJECT_CI, present=False) if plan.state == ForkState.CLOSED: comment = inactivity_close_comment(days=config.close_after_days, fork=pr.head_repo) + elif plan.state == ForkState.GATE_MODIFIED: + comment = gate_modified_close_comment(change=None) else: # The status written with the marker links the run that bypassed the gate. bypass_url = ( @@ -444,6 +469,44 @@ def classify( plan.close = True return plan + # A pull request whose CI runs in a fork must not change the gate that sent it there. Checked + # once per head commit: the marker records a head commit as checked only when the check ran + # for it (not while `use fork ci` exempted it), and a retarget since the state began changes + # what the pull request changes. A + # maintainer's reopen of one closed for it leaves the marker at gate_modified, so reopening + # without restoring the gate closes it again; `use project ci` is the way to accept the change. + gate_checked = reason in TAMPER_CHECKED_REASONS + already_checked = ( + marker is not None + and marker.gate_checked + and marker.sha == pr.head_sha + and (pr.last_base_change_at is None or pr.last_base_change_at < marker.since) + ) + if gate_checked and not already_checked: + change = find_gate_change() + if change is not None: + plan.state = ForkState.GATE_MODIFIED + if not pr.is_draft: + plan.draft = True + _set_label(plan, labels, LABEL_READY_FOR_REVIEW, present=False) + _set_label(plan, labels, LABEL_NEEDS_PROJECT_CI, present=False) + _set_status( + plan, pr, ForkState.GATE_MODIFIED.value, f"https://github.com/{repo}/pull/{pr.number}/files"; + ) + _set_marker_comment( + plan, + pr, + state_comment( + Marker(ForkState.GATE_MODIFIED.value, pr.head_sha, now), fork=pr.head_repo, run_url=None + ), + ) + plan.new_comments.append(gate_modified_close_comment(change=change)) + plan.close = True + plan.attention.append( + f"closed for changing the fork CI gate (author @{pr.author_login}): {change}" + ) + return plan + state, unknown_reason = observe_fork_state(fork, marker) if state is None: plan.unknown.append(unknown_reason or "unknown fork state") @@ -493,7 +556,11 @@ def classify( _set_marker_comment( plan, pr, - state_comment(Marker(ForkState.CLOSED.value, pr.head_sha, now), fork=pr.head_repo, run_url=None), + state_comment( + Marker(ForkState.CLOSED.value, pr.head_sha, now, gate_checked=gate_checked), + fork=pr.head_repo, + run_url=None, + ), ) plan.new_comments.append(inactivity_close_comment(days=config.close_after_days, fork=pr.head_repo)) plan.close = True @@ -518,7 +585,11 @@ def classify( _set_marker_comment( plan, pr, - state_comment(Marker(state.value, pr.head_sha, since), fork=pr.head_repo, run_url=fork.run_url), + state_comment( + Marker(state.value, pr.head_sha, since, gate_checked=gate_checked), + fork=pr.head_repo, + run_url=fork.run_url, + ), ) return plan @@ -626,11 +697,10 @@ def parse_pull_request(node: dict[str, Any]) -> PullRequestSnapshot: # and reopening their own pull request must not hide a tampered run from it. if _login(item.get("actor")).casefold() != _login(node.get("author")).casefold(): last_reopened = last_reopened_by_anyone - elif ( - item["__typename"] == "UnlabeledEvent" - and (item.get("label") or {}).get("name") == LABEL_USE_PROJECT_CI - ): - last_unlabel = parse_github_datetime(item["createdAt"]) + elif item["__typename"] == "UnlabeledEvent": + label = (item.get("label") or {}).get("name") + if label == LABEL_USE_PROJECT_CI: + last_unlabel = parse_github_datetime(item["createdAt"]) current_status = None status_created_at = None @@ -696,6 +766,17 @@ def _is_not_found(error: GitHubError) -> bool: return "404" in str(error) or "Not Found" in str(error) +def read_file(client: GitHubClient, repo: str, path: str, ref: str) -> str | None: + """Return a file's content at a ref, or None when it does not exist there.""" + try: + content = client.rest_get(f"repos/{repo}/contents/{path}?ref={quote(ref, safe='')}") + except GitHubError as e: + if _is_not_found(e): + return None + raise + return base64.b64decode(content["content"]).decode() + + class BaseGateConfigs: """ The gate configuration of each base branch, as the gate job of a pull request against it reads it. @@ -710,15 +791,7 @@ class BaseGateConfigs: self._configs: dict[str, GateConfig | None] = {default_branch: default_config} def _read(self, path: str, base_ref: str) -> str | None: - try: - content = self._client.rest_get( - f"repos/{self._repo}/contents/{path}?ref={quote(base_ref, safe='')}" - ) - except GitHubError as e: - if _is_not_found(e): - return None - raise - return base64.b64decode(content["content"]).decode() + return read_file(self._client, self._repo, path, base_ref) def get(self, base_ref: str) -> GateConfig | None: """Return the base branch's configuration, or None when the base branch has no gate.""" @@ -1042,6 +1115,137 @@ class BypassFinder: return None +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. + + 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. + """ + if workflow_text is None: + return None + try: + workflow = yaml.safe_load(workflow_text) + except yaml.YAMLError: + return object() + jobs = workflow.get("jobs") if isinstance(workflow, dict) else None + if not isinstance(jobs, dict): + return object() + build_info = jobs.get("build-info") + build_info = build_info if isinstance(build_info, dict) else {} + return ( + jobs.get("gate"), + build_info.get("needs"), + build_info.get("if"), + find_jobs_outside_gate(jobs), + workflow.get("on", workflow.get(True)), + workflow.get("defaults"), + workflow.get("env"), + workflow.get("permissions"), + workflow.get("concurrency"), + ) + + +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.""" + + 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) + ) + + return frozenset(job_id for job_id in jobs if job_id != "gate" and not depends_on_gate(job_id)) + + +def is_own_pilot_change(base_text: str | None, head_text: str | None, author_login: str) -> bool: + """Whether a gate configuration differs from the base only in the author's own pilot entry.""" + if base_text is None or head_text is None: + return False + try: + base = parse_config(base_text) + head = parse_config(head_text) + except (ValueError, TypeError, AttributeError): + # Valid TOML of the wrong shape, such as `pilot = []`, is no permitted change either. + return False + base_pilot = {login.casefold(): joined for login, joined in base.pilot.items()} + head_pilot = {login.casefold(): joined for login, joined in head.pilot.items()} + changed = { + login + for login in base_pilot.keys() | head_pilot.keys() + if base_pilot.get(login) != head_pilot.get(login) + } + 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 GateChangeFinder: + """Finds how a pull request changes the fork CI gate, from its changed files.""" + + def __init__(self, client: GitHubClient, *, repo: str): + self._client = client + self._repo = repo + + def _merge_base(self, pr: PullRequestSnapshot) -> str: + # The pull request's changes are against the merge base, not the base branch's tip, which + # may have moved on: comparing with the tip would count others' later changes as its own. + owner = (pr.head_repo or "").split("/", 1)[0] + compare = self._client.rest_get( + f"repos/{self._repo}/compare/{quote(pr.base_ref, safe='')}...{owner}:{pr.head_sha}?per_page=1" + ) + return compare["merge_base_commit"]["sha"] + + 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"] + if current_head != pr.head_sha: + raise HeadMovedError( + f"head moved from {pr.head_sha[:8]} to {current_head[:8]} 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" + paths = set(changed_paths(files)) + for path in sorted(GATE_PROTECTED_PATHS & paths): + return f"it changes `{path}`" + if not paths & {GATE_CONFIG_PATH, CI_WORKFLOW_PATH}: + return None + head_repo = pr.head_repo or "" + merge_base = self._merge_base(pr) + if GATE_CONFIG_PATH in paths and not is_own_pilot_change( + read_file(self._client, self._repo, GATE_CONFIG_PATH, merge_base), + read_file(self._client, head_repo, GATE_CONFIG_PATH, pr.head_sha), + 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)): + 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" + ) + return None + + COMMENTS_QUERY = """ query($owner: String!, $name: String!, $number: Int!, $cursor: String) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { @@ -1218,6 +1422,7 @@ def run_reconcile( ) shared_heads = find_heads_shared_across_bases(prs) + gate_change_finder = GateChangeFinder(client, repo=repo) for listed in prs: try: pr = listed if listed.comments_cursor is None else find_state_comment(client, repo, listed) @@ -1239,10 +1444,13 @@ def run_reconcile( 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, + find_gate_change=functools.partial(gate_change_finder.find, pr), ) apply_plan(client, pr, plan) unknown.extend((pr.number, reason) for reason in plan.unknown) attention.extend((pr.number, item) for item in plan.attention) + except HeadMovedError 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}[/]") unknown.append((listed.number, f"{type(e).__name__}: {e}")) diff --git a/dev/breeze/tests/test_fork_ci_messages.py b/dev/breeze/tests/test_fork_ci_messages.py index efa4ac9af9c..33bf00d6b4d 100644 --- a/dev/breeze/tests/test_fork_ci_messages.py +++ b/dev/breeze/tests/test_fork_ci_messages.py @@ -16,6 +16,7 @@ # under the License. from __future__ import annotations +import dataclasses from datetime import UTC, datetime import pytest @@ -25,6 +26,7 @@ from airflow_breeze.utils.fork_ci_messages import ( _STATUS_DESCRIPTIONS, FOOTER, Marker, + gate_modified_close_comment, inactivity_close_comment, nudge_comment, parse_marker, @@ -32,6 +34,7 @@ from airflow_breeze.utils.fork_ci_messages import ( render_marker, state_comment, tamper_close_comment, + ungated_comment, ) MARKER = Marker(state="awaiting_push", sha="abc123", since=datetime(2026, 10, 15, 10, 0, tzinfo=UTC)) @@ -39,6 +42,8 @@ MARKER = Marker(state="awaiting_push", sha="abc123", since=datetime(2026, 10, 15 def test_marker_round_trip(): assert parse_marker(f"text before\n{render_marker(MARKER)}\ntext after") == MARKER + checked = dataclasses.replace(MARKER, gate_checked=True) + assert parse_marker(render_marker(checked)) == checked def test_parse_marker_absent(): @@ -73,6 +78,8 @@ def test_parse_marker_malformed(body): pytest.param(nudge_comment(fork="u/airflow"), id="nudge"), pytest.param(inactivity_close_comment(days=7, fork="u/airflow"), id="inactivity"), pytest.param(tamper_close_comment(run_url="https://x";), id="tamper"), + pytest.param(gate_modified_close_comment(change="it changes `x`"), id="gate-modified"), + pytest.param(ungated_comment(MARKER, base_ref="v2-10-test"), id="ungated"), ], ) def test_every_comment_ends_with_footer(text): diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index c01d28c4ab5..2182989d2b3 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -34,6 +34,8 @@ from airflow_breeze.utils.fork_ci_reconcile import ( BypassFinder, ForkObservation, ForkState, + GateChangeFinder, + HeadMovedError, PullRequestSnapshot, apply_plan, build_slack_payload, @@ -44,6 +46,7 @@ from airflow_breeze.utils.fork_ci_reconcile import ( fetch_workflow_state, find_heads_shared_across_bases, find_state_comment, + is_own_pilot_change, jobs_show_project_ci, load_workflow_triggers, parse_pull_request, @@ -80,12 +83,19 @@ def _pr(**overrides) -> PullRequestSnapshot: return PullRequestSnapshot(**values) -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 _marker(state: str, *, days_ago: float = 0, sha: str = "abc123", gate_checked: bool = False) -> Marker: + return Marker(state=state, sha=sha, since=NOW - timedelta(days=days_ago), gate_checked=gate_checked) def _classify( - pr, fork=None, *, bypass_run=None, changes_ci_files=False, config=CONFIG, head_shared_across_bases=False + pr, + fork=None, + *, + bypass_run=None, + changes_ci_files=False, + config=CONFIG, + head_shared_across_bases=False, + gate_change=None, ): find_bypass = mock.Mock(return_value=bypass_run) changes = mock.Mock(return_value=changes_ci_files) @@ -98,6 +108,7 @@ def _classify( find_bypass_run=find_bypass, changes_ci_files=changes, head_shared_across_bases=head_shared_across_bases, + find_gate_change=lambda: gate_change, ) return plan, find_bypass, changes @@ -451,7 +462,7 @@ def test_unfinished_tampering_closure_is_retried(pr_overrides, expected_link): def test_since_is_kept_while_state_is_unchanged(): - marker = Marker("setup_required", "abc123", NOW - timedelta(days=2)) + marker = Marker("setup_required", "abc123", NOW - timedelta(days=2), gate_checked=True) plan, _, _ = _classify(_pr(marker=marker), ForkObservation(workflow_state="disabled_fork")) @@ -889,6 +900,11 @@ def test_parse_pull_request_reads_timeline_status_and_marker(): "createdAt": "2026-11-03T00:00:00Z", "label": {"name": "use project ci"}, }, + { + "__typename": "UnlabeledEvent", + "createdAt": "2026-11-03T12:00:00Z", + "label": {"name": "use fork ci"}, + }, { "__typename": "ConvertToDraftEvent", "createdAt": "2026-11-04T00:00:00Z", @@ -1171,6 +1187,10 @@ def test_run_reconcile_isolates_failures_and_reports_them(client, tmp_path): if len(workflow_state_calls) == 1: raise GitHubError("gh api failed: HTTP 500") return {"state": "disabled_fork"} + if path.endswith("/files?per_page=100"): + return [] + if path.startswith(f"repos/{REPO}/pulls/"): + return {"head": {"sha": f"sha{path.rsplit('/', 1)[1]}"}} raise AssertionError(path) client.rest_get.side_effect = rest_get @@ -1323,3 +1343,288 @@ def test_run_reconcile_tells_the_finder_which_bases_are_gated( client.rest_get.side_effect = rest_get assert mock_finder.call_args.kwargs["is_gated_base"](base_ref) is expected + + +def test_a_change_to_the_gate_closes(): + pr = _pr(is_draft=False, labels=("fork ci required", "ready for maintainer review")) + + plan, _, _ = _classify(pr, GREEN, gate_change="it changes `x`") + + assert plan.state == ForkState.GATE_MODIFIED + assert plan.close is True + assert plan.draft is True + assert plan.remove_labels == ["ready for maintainer review"] + assert plan.status == ( + "error", + "https://github.com/apache/airflow/pull/42/files";, + "Pull request changes the fork CI gate - see the comment", + ) + assert "changes the fork CI gate: it changes `x`" in plan.new_comments[0] + assert '"state": "gate_modified"' in plan.marker_comment + assert len(plan.attention) == 1 + + [email protected]( + ("pr_overrides", "checked"), + [ + pytest.param({}, True, id="new-pull-request"), + pytest.param({"marker": _marker("running", sha="older")}, True, id="new-head-commit"), + pytest.param( + {"marker": _marker("running", gate_checked=True)}, False, id="head-commit-already-checked" + ), + pytest.param({"marker": _marker("running")}, True, id="marker-from-before-the-check"), + pytest.param( + {"marker": _marker("gate_modified", days_ago=1), "last_reopened_at": NOW}, + True, + id="reopened-by-maintainer", + ), + pytest.param( + {"author_association": "MEMBER", "labels": ("fork ci required", "use fork ci")}, + False, + id="committer-on-fork-ci", + ), + pytest.param({"marker": _marker("released")}, True, id="released-without-a-check"), + pytest.param({"marker": _marker("ungated")}, True, id="ungated-without-a-check"), + pytest.param( + { + "marker": _marker("running", days_ago=1, gate_checked=True), + "last_base_change_at": NOW - timedelta(hours=1), + }, + True, + id="retargeted-since", + ), + ], +) +def test_gate_change_is_checked_once_per_head_commit(pr_overrides, checked): + find = mock.Mock(return_value=None) + classify( + _pr(**pr_overrides), + GREEN, + config=CONFIG, + now=NOW, + repo=REPO, + find_bypass_run=lambda: None, + changes_ci_files=lambda: False, + find_gate_change=find, + ) + + assert find.called is checked + + +def test_unfinished_gate_change_closure_is_retried(): + plan, _, _ = _classify(_pr(marker=_marker("gate_modified", days_ago=1)), GREEN, gate_change=None) + + assert plan.state == ForkState.GATE_MODIFIED + assert plan.close is True + assert "Closing this pull request because it changes the fork CI gate." in plan.new_comments[0] + + +PILOT_BASE = "enabled = true\n[pilot]\nother = 2026-10-01T00:00:00Z\n" + + [email protected]( + ("head_text", "expected"), + [ + pytest.param( + "enabled = true\n[pilot]\nother = 2026-10-01T00:00:00Z\nSomeone = 2026-12-01T00:00:00Z\n", + True, + id="adds-own-entry", + ), + pytest.param("enabled = true\n", False, id="removes-another-entry"), + pytest.param( + "enabled = false\n[pilot]\nother = 2026-10-01T00:00:00Z\nsomeone = 2026-12-01T00:00:00Z\n", + False, + id="also-switches-the-gate-off", + ), + pytest.param("enabled = [", False, id="unparsable"), + pytest.param("pilot = []\n", False, id="wrong-shape"), + pytest.param(None, False, id="deleted"), + ], +) +def test_is_own_pilot_change(head_text, expected): + assert is_own_pilot_change(PILOT_BASE, head_text, "someone") is expected + + +CI_AMD = """ +on: [push, pull_request] +jobs: + gate: + steps: [{run: gate}] + build-info: + needs: [gate] + if: needs.gate.outputs.project-ci == 'true' + tests: + needs: [build-info] + steps: [{run: tests}] +""" + + +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" +) + + +def _gate_change_finder(client, changed, head_files, base_files=None, head_sha="abc123"): + base_files = { + ".github/workflows/ci-amd.yml": CI_AMD, + ".github/fork-ci-gate.toml": PILOT_BASE, + **(base_files or {}), + } + + def rest_get(path, paginate=False, items_key=None): + if path.endswith("/files?per_page=100"): + return changed + if "/compare/" in path: + return {"merge_base_commit": {"sha": "merge-base"}} + if path == f"repos/{REPO}/pulls/42": + return {"head": {"sha": head_sha}} + 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 + if name not in files: + raise GitHubError("gh api failed: HTTP 404: Not Found") + return {"content": base64.b64encode(files[name].encode()).decode()} + + client.rest_get.side_effect = rest_get + return GateChangeFinder(client, repo=REPO) + + [email protected]( + ("changed", "head_files", "expected"), + [ + pytest.param( + [{"filename": "dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py"}], + {}, + "it changes `dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py`", + id="gate-code", + ), + pytest.param( + [{"filename": "docs/x.yml", "previous_filename": ".github/workflows/fork-ci-monitor.yml"}], + {}, + "it changes `.github/workflows/fork-ci-monitor.yml`", + id="monitor-moved-away", + ), + pytest.param( + [{"filename": ".github/fork-ci-gate.toml"}], + {".github/fork-ci-gate.toml": "enabled = false\n"}, + "it changes `.github/fork-ci-gate.toml` beyond the author's own `[pilot]` entry", + id="gate-config", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + {".github/workflows/ci-amd.yml": CI_AMD.replace("[gate]", "[]")}, + GATE_JOB_CHANGE, + id="gate-unwired", + ), + pytest.param( + [{"filename": ".github/fork-ci-gate.toml"}, {"filename": ".github/workflows/ci-amd.yml"}], + { + ".github/fork-ci-gate.toml": ( + "enabled = true\n[pilot]\nother = 2026-10-01T00:00:00Z\nsomeone = 2026-12-01T00:00:00Z\n" + ), + ".github/workflows/ci-amd.yml": CI_AMD.replace("run: tests", "run: more-tests"), + }, + None, + id="own-pilot-entry-and-other-ci-change", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + {".github/workflows/ci-amd.yml": "jobs: [broken"}, + GATE_JOB_CHANGE, + id="workflow-unparsable", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + {".github/workflows/ci-amd.yml": "jobs: [one, two]\n"}, + GATE_JOB_CHANGE, + id="jobs-not-a-mapping", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + {".github/workflows/ci-amd.yml": "defaults:\n run:\n working-directory: /tmp\n" + CI_AMD}, + GATE_JOB_CHANGE, + id="workflow-defaults", + ), + pytest.param( + [{"filename": ".github/workflows/ci-amd.yml"}], + {".github/workflows/ci-amd.yml": CI_AMD.replace("needs: [build-info]", "needs: []")}, + 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( + "[push, pull_request]", "[push, pull_request, issue_comment]" + ) + }, + GATE_JOB_CHANGE, + id="trigger-added", + ), + pytest.param( + [{"filename": f"airflow-core/f{i}.py"} for i in range(3000)], + {}, + "it changes 3000 or more files, more than GitHub lists, so the gate cannot be checked", + id="listing-at-cap", + ), + ], +) +def test_gate_change_finder(client, changed, head_files, expected): + finder = _gate_change_finder(client, changed, head_files) + + assert finder.find(_pr()) == expected + + [email protected]( + ("pr_overrides", "gate_checked"), + [ + pytest.param({}, True, id="checked"), + pytest.param( + {"author_association": "MEMBER", "labels": ("fork ci required", "use fork ci")}, + False, + id="exempt-with-use-fork-ci", + ), + ], +) +def test_marker_records_whether_the_gate_was_checked(pr_overrides, gate_checked): + plan, _, _ = _classify(_pr(**pr_overrides), ForkObservation(suite_status="IN_PROGRESS", run_url=RUN_URL)) + + 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") + + with pytest.raises(HeadMovedError, match="head moved from abc123 to newer"): + finder.find(_pr()) + + +def test_run_reconcile_retries_a_pull_request_whose_head_moved(client, tmp_path): + output = tmp_path / "slack-message.json" + client.graphql.side_effect = [ + _search_result(), + _search_result(), + _search_result(_pr_node(1)), + {"pr0": {"object": None}}, + ] + client.rest_get_pages.return_value = [{"total_count": 0, "workflow_runs": []}] + + def rest_get(path, paginate=False, items_key=None): + if path.endswith("/actions/workflows/ci-amd.yml"): + return {"state": "active"} + if path == f"repos/{REPO}/pulls/1/files?per_page=100": + return [] + if path == f"repos/{REPO}/pulls/1": + return {"head": {"sha": "moved"}} + raise AssertionError(path) + + client.rest_get.side_effect = rest_get + + has_alerts = run_reconcile( + client, CONFIG, now=NOW, run_url="https://run";, output_file=output, workflows_dir=tmp_path + ) + + assert has_alerts is False + client.set_status.assert_not_called() + client.create_comment.assert_not_called() diff --git a/scripts/ci/prek/check_ci_workflows_in_sync.py b/scripts/ci/prek/check_ci_workflows_in_sync.py index 0332b150d72..b3394f8af87 100755 --- a/scripts/ci/prek/check_ci_workflows_in_sync.py +++ b/scripts/ci/prek/check_ci_workflows_in_sync.py @@ -165,7 +165,9 @@ AMD_ONLY_BLOCK = """ schedule: 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. + # 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). name: "Fork CI gate" runs-on: ["ubuntu-slim"] timeout-minutes: 5
