You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by po...@apache.org on 2026/10/17 01:32:45 UTC
(airflow) branch fork-ci-6-reconciler-followups created (now 5ffe91d7c87)
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a change to branch fork-ci-6-reconciler-followups
in repository https://gitbox.apache.org/repos/asf/airflow.git
at 5ffe91d7c87 Close fork CI pull requests that change the fork CI gate
This branch includes the following new commits:
new 01cb999b2e3 Retry the fork CI push-again nudge when posting it fails
new e3a1772093b Ignore fork-only workflows that skipped every job in the fork CI tampering check
new 7496b74bf62 Keep fork CI pull requests sharing a head commit across bases for a maintainer
new bb9616cd47e Attribute fork CI runs to the right pull request in the tampering check
new 5ffe91d7c87 Close fork CI pull requests that change the fork CI gate
The 5 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(airflow) 03/05: Keep fork CI pull requests sharing a head commit across bases for a maintainer
Posted by po...@apache.org.
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 <ja...@potiuk.com>
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"),
[
(airflow) 05/05: Close fork CI pull requests that change the fork CI gate
Posted by po...@apache.org.
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 <ja...@potiuk.com>
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
+
+
+@pytest.mark.parametrize(
+ ("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"
+
+
+@pytest.mark.parametrize(
+ ("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)
+
+
+@pytest.mark.parametrize(
+ ("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
+
+
+@pytest.mark.parametrize(
+ ("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
(airflow) 01/05: Retry the fork CI push-again nudge when posting it fails
Posted by po...@apache.org.
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 01cb999b2e315d4a503caa3482b10cdce689732b
Author: Jarek Potiuk <ja...@potiuk.com>
AuthorDate: Sat Oct 17 02:57:45 2026 +0200
Retry the fork CI push-again nudge when posting it fails
The state comment is what the next reconciler run compares with. Recording
awaiting_push there before the nudge was posted meant a failed post was never
retried, and the contributor got no prompt before the inactivity closure.
Generated-by: Claude Opus 5
---
dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md | 3 +++
dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py | 9 +++++----
dev/breeze/tests/test_fork_ci_reconcile.py | 12 ++++++++++++
3 files changed, 20 insertions(+), 4 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 3b14a552d1a..540000aab23 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
@@ -444,6 +444,9 @@ Rules that sit under the table:
closing comment or close failed, and the next run finishes the closure. The same holds for a
`gate_bypassed` marker, where only a reopen by someone other than the author counts, so the
closure is finished even after the offending run has left the tampering check's window.
+- **New comments go first.** A nudge, a closing explanation or a tampering notice is posted
+ before the state comment records the new state, so when posting fails the next run sees the
+ old state and posts it again; a repeat is preferred to a lost notice.
- **`fork ci required` goes last.** When a run removes the label (on release, or for an
ungated base branch), it does so after every other change, so a failure on the way leaves
the label in place and the next run retries the rest.
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 db21caaef39..22ec0e15b2b 100644
--- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py
+++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py
@@ -471,8 +471,6 @@ def classify(
plan.state = state
since = marker.since if marker and marker.state == state.value else now
- # The marker records awaiting_push before the nudge is posted, so a failed post is not retried;
- # tracked at https://github.com/apache/airflow/issues/74908
if marker and marker.state == ForkState.SETUP_REQUIRED.value and state == ForkState.AWAITING_PUSH:
plan.new_comments.append(nudge_comment(fork=pr.head_repo))
@@ -1012,6 +1010,11 @@ def apply_plan(client: GitHubClient, pr: PullRequestSnapshot, plan: Plan) -> Non
for label in plan.remove_labels:
if label != LABEL_FORK_CI_REQUIRED:
client.remove_label(pr.number, label)
+ # New comments go before the state comment records the new state: the state recorded there is
+ # what the next run compares with, so a failure on the way posts them again rather than never.
+ # They carry no marker, so the state comment stays the earliest bot comment with one.
+ for comment in plan.new_comments:
+ client.create_comment(pr.number, comment)
if plan.marker_comment is not None:
if pr.marker_comment_id is not None:
client.update_comment(pr.number, pr.marker_comment_id, plan.marker_comment)
@@ -1019,8 +1022,6 @@ def apply_plan(client: GitHubClient, pr: PullRequestSnapshot, plan: Plan) -> Non
client.create_comment(pr.number, plan.marker_comment)
if plan.status is not None:
client.set_status(pr.number, pr.head_sha, *plan.status)
- for comment in plan.new_comments:
- client.create_comment(pr.number, comment)
if plan.draft is False:
client.mark_ready(pr.number, pr.node_id)
if plan.close:
diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py
index 06f77699a2f..e7e929917a9 100644
--- a/dev/breeze/tests/test_fork_ci_reconcile.py
+++ b/dev/breeze/tests/test_fork_ci_reconcile.py
@@ -1189,3 +1189,15 @@ def test_slack_payload_stays_within_slack_limits():
assert "…" in texts[1]
assert texts[-1].startswith("_")
assert "more sections are in the <https://run|run log>" in texts[-1]
+
+
+def test_nudge_is_posted_before_the_state_comment_records_awaiting_push(client):
+ pr = _pr(marker=_marker("setup_required"), marker_comment_id=5)
+ plan, _, _ = _classify(pr, ForkObservation(workflow_state="active"))
+ client.create_comment.side_effect = GitHubError("gh api failed: HTTP 500")
+
+ with pytest.raises(GitHubError):
+ apply_plan(client, pr, plan)
+
+ assert "Push this branch again" in client.create_comment.call_args.args[1]
+ client.update_comment.assert_not_called()
(airflow) 04/05: Attribute fork CI runs to the right pull request in the tampering check
Posted by po...@apache.org.
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 bb9616cd47ee3bccfd675abefd7af8f86a5894e5
Author: Jarek Potiuk <ja...@potiuk.com>
AuthorDate: Sat Oct 17 03:01:39 2026 +0200
Attribute fork CI runs to the right pull request in the tampering check
Runs from forks carry no pull request, so they were matched by fork branch
alone. Legitimate project CI against a base branch without the gate, made
before a retarget, or a run of another pull request from the same branch,
could close a gated pull request for gate tampering.
Generated-by: Claude Opus 5
---
.../doc/ci/08_fork_ci_for_external_contributors.md | 9 ++-
.../src/airflow_breeze/utils/fork_ci_reconcile.py | 82 +++++++++++++++++--
dev/breeze/tests/test_fork_ci_reconcile.py | 93 +++++++++++++++++++++-
3 files changed, 174 insertions(+), 10 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 0820e007ebf..d5703324a12 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
@@ -509,8 +509,13 @@ ignored, because they ran legitimately while the label was on. So are runs creat
most recent reopen of the pull request by someone other than its author, which is how a
maintainer brings back a pull request closed for tampering; reopening one whose gate is still
missing triggers a new run, which is caught again. A reopen by the author does not count, so
-closing and reopening one's own pull request cannot hide a tampered run. A run counts as a
-bypass when both signals hold:
+closing and reopening one's own pull request cannot hide a tampered run. Runs are listed per
+fork branch, because GitHub records no pull request on runs from forks. A run made while the
+pull request targeted a base branch without the gate, known from its retargets in the
+timeline, is project CI by design and ignored; its changes are compared with that base. When
+another pull request from the same branch was open within the window, a matching run could be
+that one's, so it is reported to Slack as an attention item rather than closing the pull
+request. A run counts as a bypass when both signals hold:
1. **The run did project CI work.** Either the default branch's version of the run's workflow
is not triggered by the run's event, which covers an injected workflow and a trigger the
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 faef461b4a8..81f71b19cce 100644
--- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py
+++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py
@@ -168,6 +168,8 @@ class PullRequestSnapshot:
last_reopened_at: datetime | None = None
last_reopened_by_anyone_at: datetime | None = None
last_base_change_at: datetime | None = None
+ # (changed at, previous base, new base) for each retarget, oldest first.
+ base_changes: tuple[tuple[datetime, str, str], ...] = ()
current_status: tuple[str, str, str] | None = None
status_created_at: datetime | None = None
marker: Marker | None = None
@@ -557,7 +559,7 @@ query($q: String!, $cursor: String) {
... on ConvertToDraftEvent { createdAt actor { login } }
... on UnlabeledEvent { createdAt label { name } }
... on ReopenedEvent { createdAt actor { login } }
- ... on BaseRefChangedEvent { createdAt }
+ ... on BaseRefChangedEvent { createdAt previousRefName currentRefName }
}
}
commits(last: 1) { nodes { commit {
@@ -609,11 +611,15 @@ def parse_pull_request(node: dict[str, Any]) -> PullRequestSnapshot:
last_reopened = None
last_reopened_by_anyone = None
last_base_change = None
+ base_changes = []
for item in (node.get("timelineItems") or {}).get("nodes") or []:
if item["__typename"] == "ConvertToDraftEvent":
last_draft_actor = _login(item.get("actor"))
elif item["__typename"] == "BaseRefChangedEvent":
last_base_change = parse_github_datetime(item["createdAt"])
+ base_changes.append(
+ (last_base_change, item.get("previousRefName") or "", item.get("currentRefName") or "")
+ )
elif item["__typename"] == "ReopenedEvent":
last_reopened_by_anyone = parse_github_datetime(item["createdAt"])
# Only a reopen by someone else counts for the gate tampering check: an author closing
@@ -665,6 +671,7 @@ def parse_pull_request(node: dict[str, Any]) -> PullRequestSnapshot:
last_reopened_at=last_reopened,
last_reopened_by_anyone_at=last_reopened_by_anyone,
last_base_change_at=last_base_change,
+ base_changes=tuple(base_changes),
current_status=current_status,
status_created_at=status_created_at,
marker=marker,
@@ -870,6 +877,15 @@ def load_workflow_triggers(workflows_dir: Path) -> dict[str, frozenset[str]]:
return triggers
+def base_at(pr: PullRequestSnapshot, when: datetime) -> str:
+ """The base branch the pull request had at ``when``, from its retargets."""
+ base = pr.base_ref
+ for changed_at, previous, _ in reversed(pr.base_changes):
+ if when < changed_at:
+ base = previous
+ return base
+
+
class BypassFinder:
"""
Finds project CI runs that bypassed the gate, from the runs apache/airflow recorded.
@@ -885,12 +901,23 @@ class BypassFinder:
"""
def __init__(
- self, client: GitHubClient, *, repo: str, now: datetime, base_triggers: dict[str, frozenset[str]]
+ self,
+ client: GitHubClient,
+ *,
+ repo: str,
+ now: datetime,
+ base_triggers: dict[str, frozenset[str]],
+ is_gated_base: Callable[[str], bool] = lambda base_ref: True,
):
+ """:param is_gated_base: whether a base branch has the gate; runs against one without it are project CI by design."""
self._client = client
self._repo = repo
self._now = now
self._base_triggers = base_triggers
+ self._is_gated_base = is_gated_base
+ # Pull requests whose matching run could belong to another pull request from the same
+ # branch; reported instead of closed.
+ self.ambiguous: list[tuple[int, str]] = []
since = (now - TAMPER_WINDOW).strftime("%Y-%m-%dT%H:%M:%SZ")
self._runs_by_branch: dict[tuple[str, str], list[WorkflowRun]] = {}
incomplete = []
@@ -961,11 +988,26 @@ class BypassFinder:
self._changes_workflows[key] = None if len(files) >= COMPARE_FILES_CAP else False
return self._changes_workflows[key]
+ def _other_pull_requests(self, pr: PullRequestSnapshot) -> list[int]:
+ """Other pull requests from the same branch that were open within the tampering window."""
+ owner = pr.head_repo.split("/", 1)[0] # type: ignore[union-attr]
+ pulls = self._client.rest_get(
+ f"repos/{self._repo}/pulls?head={quote(owner, safe='')}:{quote(pr.head_ref, safe='')}"
+ "&state=all&per_page=100",
+ paginate=True,
+ )
+ window_start = self._now - TAMPER_WINDOW
+ return [
+ pull["number"]
+ for pull in pulls
+ if pull["number"] != pr.number
+ and (pull.get("closed_at") is None or parse_github_datetime(pull["closed_at"]) >= window_start)
+ ]
+
def find(self, pr: PullRequestSnapshot) -> str | None:
if not pr.head_repo:
return None
- # Matching by fork branch alone also picks up runs of another pull request from that branch
- # or from before a retarget; tracked at https://github.com/apache/airflow/issues/74906
+ # Runs are listed per fork branch: GitHub records no pull request on runs from forks.
runs = self._runs_by_branch.get((pr.head_repo.casefold(), pr.head_ref), [])
# Runs before `use project ci` was removed were legitimate. Runs before a maintainer
# reopened the pull request were already acted on; reopening without restoring the gate
@@ -976,11 +1018,26 @@ class BypassFinder:
for run in sorted(runs, key=lambda r: r.created_at, reverse=True):
if not_before is not None and run.created_at <= not_before:
continue
+ base_ref = base_at(pr, run.created_at)
+ # Project CI against a base branch without the gate is what its gate job decides.
+ if not self._is_gated_base(base_ref):
+ continue
if not self._did_run_project_ci(run):
continue
# A change set too large to list fails closed: padding a tampered commit past the
# compare API's cap must not hide the workflow change.
- if self._commit_changes_workflows(run, pr.base_ref) is not False:
+ if self._commit_changes_workflows(run, base_ref) is not False:
+ others = self._other_pull_requests(pr)
+ if others:
+ self.ambiguous.append(
+ (
+ pr.number,
+ f"project CI run {run.html_url} may have bypassed the gate, but it could also "
+ f"belong to {', '.join(f'#{n}' for n in others)} from the same branch; not closed, "
+ "check it",
+ )
+ )
+ return None
return run.html_url
return None
@@ -1144,8 +1201,20 @@ def run_reconcile(
)
]
observations = fetch_fork_observations(client, prs)
+
+ def is_gated_base(base_ref: str) -> bool:
+ # A base branch whose configuration cannot be read is treated as gated.
+ try:
+ return configs.get(base_ref) is not None
+ except (GitHubError, ValueError):
+ return True
+
bypass_finder = BypassFinder(
- client, repo=repo, now=now, base_triggers=load_workflow_triggers(workflows_dir)
+ client,
+ repo=repo,
+ now=now,
+ base_triggers=load_workflow_triggers(workflows_dir),
+ is_gated_base=is_gated_base,
)
shared_heads = find_heads_shared_across_bases(prs)
@@ -1178,6 +1247,7 @@ def run_reconcile(
console_print(f"[error]#{listed.number}: {e}[/]")
unknown.append((listed.number, f"{type(e).__name__}: {e}"))
+ attention.extend(bypass_finder.ambiguous)
notes = [bypass_finder.incomplete] if bypass_finder.incomplete else []
# The full list for the run log; Slack may show only part of a very long one.
for number, reason in [*unknown, *attention]:
diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py
index e5b28b13572..c01d28c4ab5 100644
--- a/dev/breeze/tests/test_fork_ci_reconcile.py
+++ b/dev/breeze/tests/test_fork_ci_reconcile.py
@@ -651,6 +651,8 @@ def _finder(
jobs=(),
compare_files=(".github/workflows/ci-amd.yml",),
total_count=None,
+ other_pulls=(),
+ is_gated_base=lambda base_ref: True,
):
def rest_get_pages(path):
event = path.split("event=", 1)[1].split("&", 1)[0]
@@ -663,11 +665,13 @@ def _finder(
return list(jobs)
if "/compare/" in path:
return [{"filename": name} for name in compare_files]
+ if "/pulls?head=" in path:
+ return [{"number": 42, "closed_at": None}, *other_pulls]
raise AssertionError(path)
client.rest_get_pages.side_effect = rest_get_pages
client.rest_get.side_effect = rest_get
- return BypassFinder(client, repo=REPO, now=NOW, base_triggers=BASE_TRIGGERS)
+ return BypassFinder(client, repo=REPO, now=NOW, base_triggers=BASE_TRIGGERS, is_gated_base=is_gated_base)
@pytest.fixture
@@ -782,11 +786,44 @@ def test_finder_fails_closed_on_a_change_set_it_cannot_check(client):
assert finder.find(_pr()) == "https://github.com/apache/airflow/actions/runs/7"
+def test_finder_ignores_runs_made_while_the_base_had_no_gate(client):
+ finder = _finder(
+ client, [_run(7)], jobs=TAMPERED_JOBS, is_gated_base=lambda base_ref: base_ref != "v2-10-test"
+ )
+ pr = _pr(base_changes=((NOW - timedelta(minutes=30), "v2-10-test", "main"),))
+
+ assert finder.find(pr) is None
+
+
+@pytest.mark.parametrize(
+ "other_pulls",
+ [
+ pytest.param([{"number": 43, "closed_at": None}], id="open"),
+ pytest.param(
+ [{"number": 43, "closed_at": (NOW - timedelta(hours=2)).isoformat()}], id="closed-recently"
+ ),
+ ],
+)
+def test_finder_reports_a_run_another_pull_request_may_own(client, other_pulls):
+ finder = _finder(client, [_run(7)], jobs=TAMPERED_JOBS, other_pulls=other_pulls)
+
+ assert finder.find(_pr()) is None
+ assert finder.ambiguous == [
+ (
+ 42,
+ "project CI run https://github.com/apache/airflow/actions/runs/7 may have bypassed the gate, "
+ "but it could also belong to #43 from the same branch; not closed, check it",
+ )
+ ]
+
+
def test_finder_catches_a_workflow_moved_away(client):
finder = _finder(client, [_run(7)], jobs=TAMPERED_JOBS)
client.rest_get.side_effect = lambda path, paginate=False, items_key=None: (
TAMPERED_JOBS
if path.endswith("jobs?filter=latest&per_page=100")
+ else [{"number": 42, "closed_at": None}]
+ if "/pulls?head=" in path
else [{"filename": "docs/x.yml", "previous_filename": ".github/workflows/x.yml"}]
)
@@ -867,7 +904,12 @@ def test_parse_pull_request_reads_timeline_status_and_marker():
"createdAt": "2026-11-06T00:00:00Z",
"actor": {"login": "someone"},
},
- {"__typename": "BaseRefChangedEvent", "createdAt": "2026-11-07T00:00:00Z"},
+ {
+ "__typename": "BaseRefChangedEvent",
+ "createdAt": "2026-11-07T00:00:00Z",
+ "previousRefName": "v3-1-test",
+ "currentRefName": "main",
+ },
]
},
commits={
@@ -906,6 +948,7 @@ def test_parse_pull_request_reads_timeline_status_and_marker():
assert pr.last_reopened_at == datetime(2026, 11, 5, tzinfo=UTC)
assert pr.last_reopened_by_anyone_at == datetime(2026, 11, 6, tzinfo=UTC)
assert pr.last_base_change_at == datetime(2026, 11, 7, tzinfo=UTC)
+ assert pr.base_changes == ((datetime(2026, 11, 7, tzinfo=UTC), "v3-1-test", "main"),)
assert pr.current_status == ("pending", "u", "d")
assert pr.status_created_at == datetime(2026, 11, 8, tzinfo=UTC)
assert pr.marker == marker
@@ -1234,3 +1277,49 @@ def test_finder_ignores_an_added_workflow_whose_jobs_all_skipped(client):
finder = _finder(client, [run], jobs=[_job("Fork only"), _job("Also fork only")])
assert finder.find(_pr()) is None
+
+
+@mock.patch("airflow_breeze.utils.fork_ci_reconcile.BypassFinder", autospec=True)
+def test_run_reconcile_reports_ambiguous_runs(mock_finder, client, tmp_path):
+ output = tmp_path / "slack-message.json"
+ client.graphql.side_effect = [_search_result(), _search_result(), _search_result()]
+ mock_finder.return_value.incomplete = None
+ mock_finder.return_value.ambiguous = [(42, "could also belong to #43")]
+
+ has_alerts = run_reconcile(
+ client, CONFIG, now=NOW, run_url="https://run", output_file=output, workflows_dir=tmp_path
+ )
+
+ assert has_alerts is True
+ assert "|#42>: could also belong to #43" in output.read_text()
+
+
+@pytest.mark.parametrize(
+ ("base_ref", "rest_get", "expected"),
+ [
+ pytest.param("main", None, True, id="default-branch"),
+ pytest.param("v2-10-test", _contents({}), False, id="no-gate"),
+ pytest.param("v3-1-test", GitHubError("gh api failed: HTTP 500"), True, id="unreadable"),
+ pytest.param(
+ "v3-1-test", _contents({(GATE_FILES[1], "v3-1-test"): "# gate\n"}), True, id="half-gate"
+ ),
+ ],
+)
+@mock.patch("airflow_breeze.utils.fork_ci_reconcile.BypassFinder", autospec=True)
+def test_run_reconcile_tells_the_finder_which_bases_are_gated(
+ mock_finder, client, tmp_path, base_ref, rest_get, expected
+):
+ client.graphql.side_effect = [_search_result(), _search_result(), _search_result()]
+ mock_finder.return_value.incomplete = None
+ mock_finder.return_value.ambiguous = []
+ run_reconcile(
+ client,
+ CONFIG,
+ now=NOW,
+ run_url="https://run",
+ output_file=tmp_path / "s.json",
+ workflows_dir=tmp_path,
+ )
+ client.rest_get.side_effect = rest_get
+
+ assert mock_finder.call_args.kwargs["is_gated_base"](base_ref) is expected
(airflow) 02/05: Ignore fork-only workflows that skipped every job in the fork CI tampering check
Posted by po...@apache.org.
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 e3a1772093b178bcd8358dcbd6dbad12d09ad9d8
Author: Jarek Potiuk <ja...@potiuk.com>
AuthorDate: Sat Oct 17 02:58:50 2026 +0200
Ignore fork-only workflows that skipped every job in the fork CI tampering check
A workflow or trigger the base branch does not have always counted as
project CI, so a pull request adding a fork-only workflow that its jobs
guard off in apache/airflow was closed for gate tampering although nothing
ran.
Generated-by: Claude Opus 5
---
.../doc/ci/08_fork_ci_for_external_contributors.md | 8 +++--
.../src/airflow_breeze/utils/fork_ci_reconcile.py | 34 +++++++++++++++-------
dev/breeze/tests/test_fork_ci_reconcile.py | 9 +++++-
3 files changed, 37 insertions(+), 14 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 540000aab23..f5482f19b19 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
@@ -512,7 +512,10 @@ bypass when both signals hold:
1. **The run did project CI work.** Either the default branch's version of the run's workflow
is not triggered by the run's event, which covers an injected workflow and a trigger the
pull request added to an existing one (for example `pull_request` on `ci-arm.yml`, which
- has no gate), or it is a `ci-amd.yml` run that lasted more than 5 minutes and whose jobs
+ has no gate), and one of its jobs was not skipped (judged as for `ci-amd.yml` below when it
+ is that workflow, and counted as work when the jobs cannot be listed), so a fork-only
+ workflow the pull request adds and guards off in `apache/airflow` does not count; or it is
+ a `ci-amd.yml` run that lasted more than 5 minutes and whose jobs
(`GET .../actions/runs/<id>/jobs`) show more than the gate: a job other than `Fork CI gate`
that was not skipped, a missing or duplicated gate job, or a gate job that ran for more
than 5 minutes. The last case catches project CI work done in a job renamed to the gate's
@@ -789,7 +792,8 @@ The remote-parsing block currently inside `breeze ci upgrade` is factored into
CI-defining files keeps the pull request a draft, decided once per commit) and the gate
tampering check: a tampered run on an earlier commit is caught after a clean push, each
signal alone does not close, short runs and gate-only jobs do not count, an in-progress
- non-gate job, a removed, duplicated or long-running gate job and an injected workflow do,
+ non-gate job, a removed, duplicated or long-running gate job and an injected workflow with a
+ job that ran do, an added workflow whose jobs all skipped does not,
runs before the last `use project ci` removal or the last reopen are ignored, an
uncheckable 300-file change set is reported, `label_fork_ci` pull requests are exempt and
`pilot` pull requests are checked. Discovery labels a pilot participant's
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 22ec0e15b2b..7e49ed5395b 100644
--- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py
+++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py
@@ -869,8 +869,8 @@ class BypassFinder:
signals of the design must hold for a run: it did project CI work, and its own commit changes
`.github/workflows/`. A run did project CI work when the base branch's version of its
workflow is not triggered by the run's event (an injected workflow, or a trigger the pull
- request added to an existing one, such as `pull_request` on ci-arm.yml), or when it is a long
- ci-amd.yml run whose jobs show more than the gate.
+ request added to an existing one, such as `pull_request` on ci-arm.yml) and a job of it ran,
+ or when it is a long ci-amd.yml run whose jobs show more than the gate.
"""
def __init__(
@@ -903,21 +903,33 @@ class BypassFinder:
self._did_project_ci: dict[int, bool] = {}
self._changes_workflows: dict[tuple[str, str], bool | None] = {}
+ def _list_jobs(self, run: WorkflowRun) -> list[dict[str, Any]]:
+ return self._client.rest_get(
+ f"repos/{self._repo}/actions/runs/{run.id}/jobs?filter=latest&per_page=100",
+ paginate=True,
+ items_key="jobs",
+ )
+
def _did_run_project_ci(self, run: WorkflowRun) -> bool:
if run.id not in self._did_project_ci:
if run.event not in self._base_triggers.get(run.workflow_file, frozenset()):
- # Counts even when every job skipped; tracked at
- # https://github.com/apache/airflow/issues/74905
- result = True
+ # A workflow or trigger the base branch does not have, for example a fork-only
+ # workflow the pull request adds, did no work in apache/airflow if its jobs all
+ # skipped there. Jobs that cannot be listed count as work.
+ try:
+ jobs = self._list_jobs(run)
+ except GitHubError:
+ result = True
+ else:
+ result = (
+ jobs_show_project_ci(jobs, self._now)
+ if run.workflow_file == CI_WORKFLOW_FILE
+ else any(job.get("conclusion") != "skipped" for job in jobs)
+ )
elif run.workflow_file != CI_WORKFLOW_FILE or not run.is_long_running(self._now):
result = False
else:
- jobs = self._client.rest_get(
- f"repos/{self._repo}/actions/runs/{run.id}/jobs?filter=latest&per_page=100",
- paginate=True,
- items_key="jobs",
- )
- result = jobs_show_project_ci(jobs, self._now)
+ result = jobs_show_project_ci(self._list_jobs(run), self._now)
self._did_project_ci[run.id] = result
return self._did_project_ci[run.id]
diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py
index e7e929917a9..059da404cac 100644
--- a/dev/breeze/tests/test_fork_ci_reconcile.py
+++ b/dev/breeze/tests/test_fork_ci_reconcile.py
@@ -723,7 +723,7 @@ def test_finder_ignores_legitimate_runs(client, runs, jobs, compare_files, pr_ov
],
)
def test_finder_catches_triggers_the_base_branch_does_not_have(client, run):
- finder = _finder(client, [run])
+ finder = _finder(client, [run], jobs=[_job("Tests", "success")])
assert finder.find(_pr()) == "https://github.com/apache/airflow/actions/runs/9"
@@ -1201,3 +1201,10 @@ def test_nudge_is_posted_before_the_state_comment_records_awaiting_push(client):
assert "Push this branch again" in client.create_comment.call_args.args[1]
client.update_comment.assert_not_called()
+
+
+def test_finder_ignores_an_added_workflow_whose_jobs_all_skipped(client):
+ run = _run(9, path=".github/workflows/fork-only.yml", minutes=1)
+ finder = _finder(client, [run], jobs=[_job("Fork only"), _job("Also fork only")])
+
+ assert finder.find(_pr()) is None