From 154ecc4d2cdfce2af5f20cf441bba55b3a0f51a7 Mon Sep 17 00:00:00 2001 From: Shahar Epstein <60007259+shahar1@users.noreply.github.com> Date: Wed, 17 Jun 2026 08:13:33 +0300 Subject: [PATCH] Instruct agents to reduce direct AirflowException usage Make explicit on both the authoring and review sides that direct AirflowException raises are being reduced, not added. New direct usages are disallowed (already enforced by the check-no-new-airflow-exceptions prek hook), and the reviewer guidance clarifies that the only acceptable movement of an AirflowException line is relocating an already-existing one verbatim during a refactor, which is not a new usage. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/instructions/code-review.instructions.md | 2 +- AGENTS.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index 08eec81085d32..da2674c4c1a32 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -32,7 +32,7 @@ Use these rules when reviewing pull requests to the Apache Airflow repository. - **Flag any `@lru_cache(maxsize=None)`.** This creates an unbounded cache — every unique argument set is cached forever. Note: `@lru_cache()` without arguments defaults to `maxsize=128` and is fine. - **Flag any heavy import** (e.g., `kubernetes.client`) in multi-process code paths that is not behind a `TYPE_CHECKING` guard. - **Flag any file, connection, or session opened without a context manager or `try/finally`.** -- **Flag any new `raise AirflowException` usage.** The community has stopped adding new ones (enforced by the `check-no-new-airflow-exceptions` prek hook) — prefer Python's standard exceptions (`ValueError`, `TypeError`, `OSError`), or a dedicated class in the appropriate `exceptions.py`. **Do not suggest changing specific exceptions back to `AirflowException`.** +- **Flag any new `raise AirflowException` usage.** The community is reducing direct `AirflowException` usage, not increasing it; new ones are not allowed (enforced by the `check-no-new-airflow-exceptions` prek hook) — prefer Python's standard exceptions (`ValueError`, `TypeError`, `OSError`), or a dedicated class in the appropriate `exceptions.py`. **The one exception is a pure relocation: an already-existing `AirflowException` moved verbatim during a refactor (e.g. code moved between files) is not a new usage — do not flag it, but confirm the diff removes the identical raise elsewhere and leaves it otherwise unchanged.** **Do not suggest changing specific exceptions back to `AirflowException`.** ## Testing Requirements diff --git a/AGENTS.md b/AGENTS.md index 4f8b4a37d44f6..f0eb03813f4b7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -139,7 +139,7 @@ reported as such are described in "What is NOT considered a security vulnerabili - In `airflow-core`, functions with a `session` parameter must not call `session.commit()`. Use keyword-only `session` parameters. - Imports at top of file. Valid exceptions: circular imports, lazy loading for worker isolation, `TYPE_CHECKING` blocks. - Guard heavy type-only imports (e.g., `kubernetes.client`) with `TYPE_CHECKING` in multi-process code paths. -- Define dedicated exception classes or use existing exceptions such as `ValueError` instead of raising the broad `AirflowException` directly. Each error case should have a specific exception type that conveys what went wrong. +- Define dedicated exception classes or use existing exceptions such as `ValueError` instead of raising the broad `AirflowException` directly. Each error case should have a specific exception type that conveys what went wrong. **Never add new direct `raise AirflowException(...)` usages — the community is actively reducing them, not adding more, and the `check-no-new-airflow-exceptions` prek hook enforces this across `airflow-core`, `airflow-ctl`, `task-sdk`, `providers`, and `shared`.** Prefer a Python built-in (`ValueError`, `TypeError`, `OSError`, …) or a dedicated class in the appropriate `exceptions.py`. The only acceptable way an `AirflowException` line may move is relocating an already-existing one verbatim during a refactor (e.g. moving code between files) — that is not a new usage. When you touch code that already raises `AirflowException`, prefer narrowing it to a more specific exception rather than leaving or duplicating it. - Translate domain-layer exceptions to `HTTPException` at FastAPI route boundaries. In `airflow-core/src/airflow/core_api/` route handlers, catch errors raised by domain code (e.g., `ValueError` from `airflow.state.metastore.MetastoreStateBackend` for a missing row or invalid input) and re-raise as `HTTPException` with the right status (`404` for not-found, `400` for invalid input). Otherwise they propagate as `500 Internal Server Error`, leaking internals and misleading clients. - Bulk `DELETE`/`UPDATE` in the scheduler loop or any synchronous interval task (e.g. `call_regular_interval` callbacks) must be batched with `LIMIT` and committed between batches — never issue a single unbounded bulk write against a user-driven table. Unbounded bulk writes hold row locks for the entire transaction (blocking concurrent writers) and stall the scheduler main loop. Filter columns used by the cleanup must be indexed. Follow the batching pattern in `airflow-core/src/airflow/utils/db_cleanup.py`. - Name functions and methods with action verbs: `get_`, `extract_`, `find_`, `compute_`, `build_`, etc. Avoid noun-only names like `_serialize_keys` or `_base_names` — they read as attributes, not callables. Predicates (`is_`, `has_`) are the one exception.