This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch fork-ci-gate-fail-on-unreadable-labels in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 54b3a3f2728a0a684cc087ebc6e1650a4d35f93e Author: Jarek Potiuk <[email protected]> AuthorDate: Mon Oct 26 13:23:08 2026 +0100 Approve fork CI runs that started before "use fork ci" was added When a committer adds "use fork ci" right after a PR opens, the PR's first runs were created without it, and the workflow-run approver left them for a maintainer. It had to: the fork CI gate fell back to the labels of the run's event when it could not read the current ones, and those labels could still send the run to full project CI on code nobody had reviewed. Anyone able to exhaust the shared token quota could force that fallback. The gate now fails when it cannot read the labels, so a run only ever acts on the PR's current labels and a re-run decides again. With that, runs started before the label changed are as safe to approve as later ones. Generated-by: Claude Opus 5 --- .github/workflows/ci-amd.yml | 1 - dev/breeze/doc/ci/05_workflows.md | 6 +-- .../doc/ci/08_fork_ci_for_external_contributors.md | 15 +++++--- .../src/airflow_breeze/utils/fork_ci_gate.py | 20 +++++++--- .../airflow_breeze/utils/workflow_run_approval.py | 13 ++----- dev/breeze/tests/test_fork_ci_gate.py | 45 ++++++++++------------ dev/breeze/tests/test_workflow_run_approval.py | 13 +++---- scripts/ci/prek/check_ci_workflows_in_sync.py | 1 - 8 files changed, 56 insertions(+), 58 deletions(-) diff --git a/.github/workflows/ci-amd.yml b/.github/workflows/ci-amd.yml index e99af802e95..49456595214 100644 --- a/.github/workflows/ci-amd.yml +++ b/.github/workflows/ci-amd.yml @@ -233,7 +233,6 @@ jobs: PR_AUTHOR: ${{ github.event.pull_request.user.login }} PR_AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} PR_CREATED_AT: ${{ github.event.pull_request.created_at }} - PR_EVENT_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} # A base branch with neither gate file predates the gate (the pull request adding it, # or a release branch cut before it) and keeps running project CI. With only one of # them present, the missing file fails the job instead of opening the gate. diff --git a/dev/breeze/doc/ci/05_workflows.md b/dev/breeze/doc/ci/05_workflows.md index c06d4f8a267..782c8dd3c80 100644 --- a/dev/breeze/doc/ci/05_workflows.md +++ b/dev/breeze/doc/ci/05_workflows.md @@ -138,9 +138,9 @@ run started before a label that changes which tests run was added or removed (`full tests needed`, `all versions`, `default versions only`, `latest versions only`, `canary`, `upgrade to newer dependencies`, `area:e2e-tests`, `area:kubernetes-tests`) is left for a maintainer, since it still runs the selection it started with; the next push is decided afresh. -For a PR whose CI runs in the fork the selection does not matter, but the gate falls back to the -labels a run started with when it cannot read the current ones, so a run started before -`use fork ci` or `use project ci` was added or removed is left for a maintainer instead. Other PRs get the +For a PR whose CI runs in the fork neither the selection nor the labels a run started with matter: +the gate reads the current labels when the run starts and fails when it cannot, so a run started +before `use fork ci` or `use project ci` was added or removed is approved too. Other PRs get the `need workflow approval` label and wait for a maintainer. When processing a PR fails (for example on a GitHub API error), the workflow tries to add the label to it and the workflow run fails, so the failure stays visible even if the label could not be added. The label is removed when a later commit 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 5f58617cf56..85eb012b520 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 @@ -310,9 +310,11 @@ directly with the runner's `python3` without installing breeze. The reconciler i full project CI as `base_without_gate`. 2. For pull requests, one read-only API call for the current labels (`gh api repos/apache/airflow/pulls/<n> --jq '.labels[].name'`), so override labels added - after the event are seen on a re-run. If that call fails, the gate uses the labels of the - event payload instead. It never falls back to running project CI: anyone able to exhaust - the shared token quota would otherwise switch the gate off for every pull request. + after the event are seen on a re-run. If that call fails, the gate job fails and nothing + after it runs; re-running the job decides again. It falls back neither to running project + CI, since anyone able to exhaust the shared token quota would otherwise switch the gate off + for every pull request, nor to the labels of the event payload, since runs started before + `use fork ci` was added are approved automatically (see "Approving gated runs"). 3. `python3 dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py` with the event payload fields and the labels passed as environment variables. It writes `project-ci=true|false` and `reason=...` to `GITHUB_OUTPUT`. A base branch with neither file skips the script and @@ -367,9 +369,10 @@ applies instead: `github-actions[bot]` with the marker count. A change to `.github/` can remove the gate or add a workflow that does run, so such a run must not start without a review. -A run started before `use fork ci` or `use project ci` was last added or removed is not approved -automatically: the gate falls back to the labels of the run's event when it cannot read the current -ones, and those can still send the run to project CI. The next push is decided afresh. +A run started before `use fork ci` or `use project ci` was last added or removed is approved too, +so a committer can add `use fork ci` after the PR's first runs were created and before they are +approved. The gate decides from the labels the PR has when the run starts, never from those of the +run's event, and fails when it cannot read them. Both rules use the same list as "Trusting a green fork run", so a gated pull request whose runs are approved automatically is also one whose green fork run is trusted, and one that changes CI is both 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 dc05a3b67a5..4816e81bfd3 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_gate.py @@ -214,6 +214,10 @@ class Decision: reason: Reason +class LabelsUnavailableError(RuntimeError): + """The current labels of a pull request could not be read.""" + + def _is_canonical_branch(ref: str) -> bool: branch = ref.removeprefix("refs/heads/") return any(pattern.match(branch) for pattern in CANONICAL_PUSH_BRANCH_PATTERNS) @@ -300,10 +304,12 @@ def evaluate_from_env(env: Mapping[str, str]) -> Decision: if event_name == "pull_request": fetched = fetch_pull_request_labels(repository, env.get("PR_NUMBER", "")) if fetched is None: - # Never fail open: anyone able to exhaust the shared token quota would otherwise switch - # the gate off for every pull request. The event's labels miss only labels added since. - print("Falling back to the labels of the triggering event", file=sys.stderr) - fetched = tuple(json.loads(env.get("PR_EVENT_LABELS") or "[]")) + # Neither project CI nor the event's labels: the workflow-run approver approves runs + # started before `use fork ci` was added, which the event's labels would send to project + # CI, and anyone able to exhaust the shared token quota could force that path. + raise LabelsUnavailableError( + f"Could not read the labels of pull request #{env.get('PR_NUMBER', '')}; re-run the job" + ) labels = fetched event = GateEvent( event_name=event_name, @@ -321,7 +327,11 @@ def evaluate_from_env(env: Mapping[str, str]) -> Decision: def main(env: Mapping[str, str] | None = None) -> int: env = os.environ if env is None else env - decision = evaluate_from_env(env) + try: + decision = evaluate_from_env(env) + except LabelsUnavailableError as e: + print(e, file=sys.stderr) + return 1 project_ci = "true" if decision.run else "false" print(f"project-ci={project_ci} reason={decision.reason.value}") _write_line("GITHUB_OUTPUT", f"project-ci={project_ci}", env) diff --git a/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py b/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py index 2f7fc1fe34b..5b0289d2e19 100644 --- a/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py +++ b/dev/breeze/src/airflow_breeze/utils/workflow_run_approval.py @@ -49,8 +49,6 @@ from airflow_breeze.utils.ci_minutes_estimate import CiMinutesEstimate, estimate from airflow_breeze.utils.console import console_print from airflow_breeze.utils.fork_ci_gate import ( GATED_REASONS, - LABEL_USE_FORK_CI, - LABEL_USE_PROJECT_CI, GateConfig, GateEvent, decide, @@ -132,11 +130,6 @@ TEST_SELECTION_LABELS = frozenset( } ) -# Labels that move a PR between project CI and fork CI. The gate reads them when it runs, but falls -# back to the labels of the run's event when it cannot, so a run started before one changed could -# still run project CI. -GATE_OVERRIDE_LABELS = frozenset({LABEL_USE_FORK_CI, LABEL_USE_PROJECT_CI}) - # Read from the trusted main checkout, which is also the base branch whose gate job decides the # PRs that are approved automatically. FORK_CI_GATE_CONFIG_PATH = AIRFLOW_ROOT_PATH / ".github" / "fork-ci-gate.toml" @@ -225,7 +218,9 @@ def decide_gated_approval(*, changed_files: list[ChangedFile]) -> ApprovalDecisi return ApprovalDecision( approve=True, reasons=["its CI runs in the contributor's fork, so its project CI run only executes the gate"], - decision_labels=GATE_OVERRIDE_LABELS, + # The gate reads `use fork ci` and `use project ci` when the run starts and fails when it + # cannot, so runs started before one of them changed are approved too. + decision_labels=frozenset(), ) @@ -655,7 +650,7 @@ def find_runs_started_for_current_pr( :param decision_labels: the labels the decision depends on. It is made from the current labels, while a run can act on those of its event: with ``full tests needed`` removed, or ``default versions only`` added to a run of all versions, the estimate is smaller than the - run, and without ``use fork ci`` in its event a run can fall back to project CI. + run. """ started_after = parse_github_time(pr["created_at"]) changes = [ diff --git a/dev/breeze/tests/test_fork_ci_gate.py b/dev/breeze/tests/test_fork_ci_gate.py index e641e45ca82..ca4194b99d2 100644 --- a/dev/breeze/tests/test_fork_ci_gate.py +++ b/dev/breeze/tests/test_fork_ci_gate.py @@ -391,28 +391,14 @@ def _outputs(env: dict[str, str]) -> list[str]: @mock.patch("airflow_breeze.utils.fork_ci_gate.subprocess.run", autospec=True) @pytest.mark.parametrize( - ("returncode", "stdout", "event_labels", "expected"), + ("stdout", "expected"), [ - pytest.param(0, "[]\n", "[]", ["project-ci=false", "reason=external"], id="external"), - pytest.param( - 0, '["use project ci"]\n', "[]", ["project-ci=true", "reason=label_project_ci"], id="labelled" - ), - pytest.param(1, "", "[]", ["project-ci=false", "reason=external"], id="gh-fails-stays-gated"), - pytest.param( - 0, "not json", "[]", ["project-ci=false", "reason=external"], id="bad-output-stays-gated" - ), - pytest.param( - 1, - "", - '["use project ci"]', - ["project-ci=true", "reason=label_project_ci"], - id="gh-fails-uses-event-labels", - ), + pytest.param("[]\n", ["project-ci=false", "reason=external"], id="external"), + pytest.param('["use project ci"]\n', ["project-ci=true", "reason=label_project_ci"], id="labelled"), ], ) -def test_main_pull_request(mock_run, gate_env, returncode, stdout, event_labels, expected): - mock_run.return_value = subprocess.CompletedProcess([], returncode, stdout=stdout, stderr="boom") - gate_env["PR_EVENT_LABELS"] = event_labels +def test_main_pull_request(mock_run, gate_env, stdout, expected): + mock_run.return_value = subprocess.CompletedProcess([], 0, stdout=stdout, stderr="") assert main(gate_env) == 0 @@ -420,15 +406,24 @@ def test_main_pull_request(mock_run, gate_env, returncode, stdout, event_labels, assert mock_run.call_args_list[0].args[0][:3] == ["gh", "api", "repos/apache/airflow/pulls/1"] [email protected]( - "airflow_breeze.utils.fork_ci_gate.subprocess.run", autospec=True, side_effect=FileNotFoundError("gh") [email protected]("airflow_breeze.utils.fork_ci_gate.subprocess.run", autospec=True) [email protected]( + ("returncode", "stdout", "side_effect"), + [ + pytest.param(1, "", None, id="gh-fails"), + pytest.param(0, "not json", None, id="bad-output"), + pytest.param(0, "", FileNotFoundError("gh"), id="no-gh"), + ], ) -def test_main_without_gh_uses_the_event_labels(mock_run, gate_env): - gate_env["PR_EVENT_LABELS"] = '["use project ci"]' +def test_main_pull_request_fails_when_labels_cannot_be_read( + mock_run, gate_env, returncode, stdout, side_effect +): + mock_run.return_value = subprocess.CompletedProcess([], returncode, stdout=stdout, stderr="boom") + mock_run.side_effect = side_effect - main(gate_env) + assert main(gate_env) == 1 - assert _outputs(gate_env) == ["project-ci=true", "reason=label_project_ci"] + assert not Path(gate_env["GITHUB_OUTPUT"]).exists() @mock.patch("airflow_breeze.utils.fork_ci_gate.subprocess.run", autospec=True) diff --git a/dev/breeze/tests/test_workflow_run_approval.py b/dev/breeze/tests/test_workflow_run_approval.py index 0877efa90d3..87e68302911 100644 --- a/dev/breeze/tests/test_workflow_run_approval.py +++ b/dev/breeze/tests/test_workflow_run_approval.py @@ -31,7 +31,6 @@ from airflow_breeze.utils.fork_ci_messages import CI_CHANGE_MARKER from airflow_breeze.utils.selective_checks import SelectiveChecks from airflow_breeze.utils.shared_options import set_dry_run from airflow_breeze.utils.workflow_run_approval import ( - GATE_OVERRIDE_LABELS, MAX_AUTO_APPROVED_JOB_MINUTES, MAX_AUTO_APPROVED_SOURCE_LINES, MAX_ERROR_TEXT_LENGTH, @@ -995,13 +994,11 @@ def test_get_last_label_change_ignores_other_labels_and_reads_the_timeline_once( @mock.patch(f"{MODULE}.load_config", autospec=True, return_value=GATE_ENABLED) @mock.patch(f"{MODULE}.get_changed_files", autospec=True, return_value=CORE_CHANGE) @mock.patch(f"{MODULE}.fetch_merge_commit", autospec=True, return_value="merge") -def test_gated_approval_depends_on_the_gate_override_labels( - mock_fetch, mock_get_changed_files, mock_load_config -): +def test_gated_approval_depends_on_no_label(mock_fetch, mock_get_changed_files, mock_load_config): api = mock.MagicMock(spec=GitHubApi) api.github_repository = "apache/airflow" - assert decide_for_pull_request(api=api, pr=_gated_pr()).decision_labels == GATE_OVERRIDE_LABELS + assert decide_for_pull_request(api=api, pr=_gated_pr()).decision_labels == frozenset() @pytest.mark.parametrize("event", ["labeled", "unlabeled"]) @@ -1016,14 +1013,14 @@ def test_get_last_label_change_counts_every_test_selection_label(label, event): @mock.patch(f"{MODULE}.decide_for_pull_request", autospec=True) -def test_process_waiting_runs_leaves_gated_runs_started_before_use_fork_ci_was_added(mock_decide): +def test_process_waiting_runs_approves_gated_runs_started_before_use_fork_ci_was_added(mock_decide): api = GitHubApi(github_repository="apache/airflow", github_token="token", report_only=False) timeline = [ {"event": "labeled", "created_at": "2026-10-22T10:30:00Z", "label": {"name": "use fork ci"}}, {"event": "unlabeled", "created_at": "2026-10-22T10:40:00Z", "label": {"name": "full tests needed"}}, ] mock_decide.return_value = ApprovalDecision( - approve=True, reasons=["fork CI"], decision_labels=GATE_OVERRIDE_LABELS + approve=True, reasons=["fork CI"], decision_labels=frozenset() ) with ( mock.patch.object( @@ -1041,4 +1038,4 @@ def test_process_waiting_runs_leaves_gated_runs_started_before_use_fork_ci_was_a ): process_waiting_runs(api, created_since=CREATED_SINCE) - assert approve_run.call_args_list == [mock.call(3)] + assert approve_run.call_args_list == [mock.call(2), mock.call(3)] diff --git a/scripts/ci/prek/check_ci_workflows_in_sync.py b/scripts/ci/prek/check_ci_workflows_in_sync.py index 321ca94194c..edee96e3a9b 100755 --- a/scripts/ci/prek/check_ci_workflows_in_sync.py +++ b/scripts/ci/prek/check_ci_workflows_in_sync.py @@ -229,7 +229,6 @@ AMD_ONLY_GATE_BLOCK = """ gate: PR_AUTHOR: ${{ github.event.pull_request.user.login }} PR_AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} PR_CREATED_AT: ${{ github.event.pull_request.created_at }} - PR_EVENT_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} # A base branch with neither gate file predates the gate (the pull request adding it, # or a release branch cut before it) and keeps running project CI. With only one of # them present, the missing file fails the job instead of opening the gate.
