[DO NOT REVIEW] Add per-area review criteria and ADRs for PR triage - #70187
Draft
potiuk wants to merge 21 commits into
Draft
[DO NOT REVIEW] Add per-area review criteria and ADRs for PR triage#70187potiuk wants to merge 21 commits into
potiuk wants to merge 21 commits into
Conversation
boring-cyborg
Bot
added
area:airflow-ctl
area:API
Maintainer review time is the scarce resource in the PR queue. This adds a pr-management-triage override that scores each PR on the imbalance between how cheap it was to produce and how expensive it is to review properly — combining per-area criticality/difficulty with the author's demonstrated standing in that area — and proposes closing, drafting back, or nudging to discuss, always maintainer-fired. The per-area AGENTS.md gain criticality/difficulty metadata and a Review criteria checklist mined from real merged and rejected PR reviews, so authoring agents can self-check before opening a PR and triage can draft back PRs that skipped it.
Critical/high areas now instruct an authoring agent to refuse to open a PR when the driving person lacks the area's required experience, and to propose a simpler issue or a better-matched area instead — the cheapest place to catch a review-cost imbalance is before the PR exists. The maintainer-already-engaged exemption now requires substantive review (an approval, change-request, or code-level comment), not a triage nudge, so a PR a committer merely touched while triaging is no longer waved through. A docstring/comment-only diff is scored as documentation regardless of which file it lives in, so a comment fix in a critical file is not treated as a high-cost change.
…nto the imbalance step The review criteria are the checklist; the ADRs are the why. Recording each area's architectural decisions (with the PR history that shaped them) lets the triage step flag a change that goes against the grain of an area — reshaping an Execution-API response with no migration, retrying past a deadlock instead of fixing lock ordering, widening a token scope in passing — not only a change that misses a checklist item. Authoring and reviewing agents read the same records, so the reasoning behind the rules travels with the code.
…ry ADR The area AGENTS.md used asterisk emphasis where the repo's markdownlint config requires underscores; normalising it lets the lint-markdown hook pass so these files don't need a verify bypass. The new Execution-API ADR records the boundary the conformance sweep surfaced — user code and SDK callbacks reach metadata state through the Execution API, never by opening a DB session directly.
Point-in-time analysis behind the branch: the full simulated-action stats over the ready and not-ready PR queues, and the ADR-conformance sweep results. Force-added past the gitignored files/ dir to preserve them with the branch; drop this commit before opening an upstream PR — these snapshots are not source-tree material.
…d task-sdk execution Extend the review-imbalance area metadata to three more architecture-boundary areas: the Dag File Processor (untrusted-code isolation, timeout-bounded parse subprocesses, versioned bundles), the Core API server (error translation at the route boundary, the generated stable v2 contract, sole DB mediator + auth dependencies), and the Task SDK execution runtime (workers reach the DB only via the Execution API, the supervised-subprocess message protocol, and independent versioning with no airflow-core ORM imports). Each area carries an AGENTS.md with mined review criteria plus numbered ADRs whose Evidence cites real merged PRs.
…imetables, triggers and secrets Extend the review-imbalance area metadata to six more areas: DB migrations (released-migration immutability, cross-backend reversibility, batched data migrations), the auth manager (authorization decided in the manager, BaseAuthManager as a stable provider-facing interface, fail-closed and security-gated), assets (stable normalized identity, serialized-data-only asset scheduling, two-sided cross-team delivery checks), timetables (deterministic next_dagrun_info, serialized form as a compatibility contract, timezone/DST correctness), triggers (async non-blocking run(), serialize to classpath+kwargs, triggerer runs user code isolated from the DB) and secrets backends (stable interface + search order, never log/leak secrets, fail-safe bounded lookups). Each area carries an AGENTS.md with mined review criteria plus numbered ADRs whose Evidence cites real merged PRs.
…d api-common Extend the review-imbalance area metadata to three more areas: the Task SDK Dag authoring surface (the public authoring API is backward-compat-sensitive, definitions are declarative and serialize losslessly, and the SDK is independent of airflow-core), task-instance dependencies (dep checks are deterministic and side-effect-free with no user code, are evaluated cheaply per task instance in the scheduling loop, and follow the stable BaseTIDep/DepContext contract), and the shared API infrastructure (bounded paginated querying is centralized in paginated_select, the helper set is a high-fan-in cross-endpoint contract, and DB session discipline plus safe filter composition live here). Each area carries an AGENTS.md with mined review criteria plus numbered ADRs whose Evidence cites real merged PRs.
Extend the review-imbalance area metadata to the shared/ libraries — the small independent distributions (logging, serialization, secrets_masker, configuration, observability, …) symlinked into airflow-core, task-sdk and providers. The area carries an AGENTS.md with mined review criteria plus three ADRs: shared libraries are independent distributions consumed via symlinks (change at source, run their own tests, one edit lands in every consumer), they must not import their consumers (the check-airflow-imports-in-shared boundary keeps the layering acyclic and the SDK installable without core), and the load-bearing libraries must fail safe (secrets_masker never under-masks, logging/observability degrade rather than crash, serialization stays deterministic and cross-version). Evidence cites real merged PRs.
…nd airflow-ctl Extend the review-imbalance area metadata to three more areas: the Task SDK operator/hook base classes (BaseOperator and friends are a stable public API that users and every provider subclass, operators are declarative at parse time and only do work at execute time, and the base classes are a provider-facing interface), the Task SDK Execution-API client (the client is the sole worker-to-server path on a task-scoped token, its datamodels are generated from the Execution-API spec and stay in server parity, and it must be resilient across independent deploys), and airflowctl (a standalone remote client that reaches Airflow only through the public REST API with no core import or DB access, whose API client is generated from the OpenAPI spec, and whose CLI/output surface is a stable contract changelogged in RELEASE_NOTES.rst). Each area carries an AGENTS.md with mined review criteria plus numbered ADRs citing real merged PRs.
…utils and prek hooks Close the remaining review-imbalance coverage gaps. Providers were the largest blind spot: ~21% of the ready-for-maintainer-review queue touches providers/ and previously fell through to the neutral medium/medium fallback with no ADR or criteria check. Also adds the Helm chart (values contract, upgrade safety, independent release), the React UI (REST-only access, generated TS client, locale catalogues), dev tooling (selective-checks logic/docs/tests move together, no release-blocking single point of failure, generated artifacts are regenerated), core utils (session lifecycle, batched bulk maintenance writes, high fan-in internal API) and the prek hooks (fast and precisely scoped, actionable and deterministic failures, a hook is the executable form of a documented rule). The four areas that already had an AGENTS.md are extended in place, preserving their existing guidance. dev/adr/ is excluded from doctoc so ADRs keep the license header first and stay consistent with the other areas' ADRs.
Provider dependencies are no longer generated from provider.yaml: no provider.yaml declares a dependencies key, and each provider's generated pyproject.toml states that dependencies are edited in place and preserved when the file is regenerated. provider.yaml remains the source of truth for provider metadata.
Running the step over a real 284-PR ready-for-maintainer-review queue surfaced five things the write-up did not cover. The small-diff exemption sets cost, not correctness: a 5-line chart template change scored low, passed the matrix, and still rolled every worker pod on upgrade with no newsfragment. Small diffs in critical areas are the highest-yield ADR targets, not the safest, so the exemption now says so explicitly. The ADR check needed two guards. A change that removes an existing violation moves with the decision and must not be flagged - several one-line fixes deleted an improper session.commit() or an unbounded query. And decisions conditional on release state must have that state verified rather than pattern-matched: two diffs rewrote a down_revision chain identically and only the one whose revisions had shipped was a retcon. The never-lowers clause was misread in practice as letting a pass or discuss survive a contradiction; it only means a conforming change earns no bonus, and draft-back supersedes both per 6.3. In-area standing is now measured with the REST commits API filtered by author and path, which scans full history on the core budget - PR search does not index file paths and silently misses in-area work. Finally, the fallback tier is a silent blind spot (uncovered areas present as everything passes), and strict in-area standing is sharp on prolific contributors - both are now documented with the measured effect so an adopter decides deliberately rather than by accident.
Records the run over the 284-PR ready-for-maintainer-review queue at 29-area coverage: the coverage delta (56% to 94%), the ReviewCost shift, dispositions under lenient vs strict in-area standing, and the first application of the ADR conformance step. Also records a simulated aggressive ruleset and the argument against adopting it: un-softening the matrix while keeping strict in-area standing turns 35 draft-backs into closes, all of them landing on contributors with substantial overall records who simply have no merged commits in the area their PR touches.
The single providers/ area cannot express the blast-radius spread inside that tree: a hook fix in one provider reaches that provider's users, while a common.compat change reaches a hundred distributions. Nearest-ancestor resolution lets each of these override the generic area with its own tier and decisions. Chosen for fan-out: common/sql (DbApiHook is imported by 29 distributions), common/compat (100 declare it as a runtime dependency), and standard (whose reach is through user Dags rather than other providers). Chosen for volume or criticality: amazon, google, fab (auth implementation, vendored Flask-AppBuilder, its own migration chain) and cncf/kubernetes (executor plus pod operator). amazon and google are deliberately medium despite being the largest by change volume - a broken S3 hook lands on that provider's users, not the cluster - while common/sql at a fifth of the volume is high because 29 independently released packages inherit its breakage. common/sql already kept ADRs under doc/adr/, so the new decisions continue that series as 0004-0006 rather than starting a competing directory.
The rule pointed at providers/fab/src/airflow-core/src/airflow/providers/fab/migrations/, which does not exist, so FAB's own alembic chain had no owner. The real path is providers/fab/src/airflow/providers/fab/migrations/.
…eview-imbalance triage The small-diff exemption was the only size signal, and its ceilings are tight enough in critical areas (10 lines, 1 file) that ordinary focused fixes -- a 4-line change across 2 files -- were scored as if they were sweeping rewrites. A second, wider band gives those changes credit for being contained without granting them the small exemption's bypass of the close path. Oversized first contributions get a bounded nudge toward being split rather than reviewed as-is, capped so that size alone can never close a newcomer's PR. Automation accounts are excluded from author standing entirely: bots have no merged-PR history and were being scored in the harshest row for it.
Re-runs the read-only simulation over the whole ready-for-maintainer-review queue with author standing resolved for every PR rather than only the escalated subset, so the disposition numbers now describe the queue instead of a sample.
Airflow's review capacity is the scarce resource, and the guidance that existed described how the code works rather than what the project has already decided. A contributor could read it, write something reasonable, and still be asked to change direction late, because the decisions being applied in review were not written down anywhere. These records are mined from the project's own history across 36 areas, including the closed-unmerged PRs that carry the reasons work gets turned away. That half was previously almost absent: the evidence base was 3 percent rejected PRs and is now 21 percent. Each decision states what a violating change looks like, so it can be checked against a diff rather than recalled. The rules were then attacked adversarially and measured against roughly 480 live open PRs. That found rules firing on work the project had already merged - an index migration that fixed a real scheduler stall, standalone cookie hardening, the removal of an N+1, every AIP-94 command port - and those have been narrowed to carry the exceptions their own reasoning already implied. Areas whose rules caught nothing are recorded as such rather than sharpened until they fire. Nothing here gates automatically. The material is written to support drafting a change back to its author with a specific reason, and every area's measured false-positive rate is stated so the limits are visible.
…urement The previous run reported dispositions without knowing how often the rules fire on work the project would have accepted. Running them against roughly 480 live open PRs answers that, and the answer changes the recommendation: the rules support drafting a change back with a reason, and do not yet support closing anything automatically. Also retracts one reported ADR violation. apache#68180 was described as flipping a released operator default; it restores a documented config key that no code reads, which is a defect fix rather than a semantic change.
potiuk
force-pushed
the
pr-review-imbalance
branch
from
July 21, 2026 12:24
e89d81d to
33e46be
Compare
potiuk
added
the
skip common compat check
The ADRs had grown to roughly twice the length a decision record needs, mostly because each Context section stated its mechanism once and then re-told it through the same example two or three more times, Consequences re-argued points the Decision already made, and every Evidence entry ran several sentences. This trims that prose — Context to the mechanism stated once, Consequences to a few bullets, Evidence to one line per PR — while leaving every Decision bullet and every violation condition and carve-out untouched. Those were tuned against live PRs to avoid firing on legitimate work, so they are frozen here: the violation blocks are byte-identical to before, save for line rewrapping. Every cited PR is retained except four duplicate backport illustrations in one file, whose point the file's counter-case already makes.
potiuk
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this exists: this is mostly an exploration of the impact that having all
these per-area ADRs and review criteria would have on the PRs people open — what
would get flagged, drafted back, or waved through, and how often the rules would
fire on work the project actually accepts. The measurement is the point; the
documents are the instrument.
What this is
Per-area review guidance for
apache/airflow: anAGENTS.mdreview-criteriachecklist and a set of Architecture Decision Records for each of 36 areas of the
repo, mined from the project's own history — both merged PRs and, importantly, the
closed-unmerged PRs that record why work gets turned away.
Each ADR states what a violating change looks like, so a decision can be checked
against a diff rather than reconstructed from memory during review. The goal is to
give a reviewer a specific, evidenced reason to draft a change back to its author.
Status / caveats (why not to review yet)
PRs. That measurement is captured in
files/pr-review-imbalance-simulation-full-coverage.md.drafting-back with a reason; the measured conclusion is that no area is safe to
drive automatic closure.
projected from the fixes applied, not yet re-measured. A second validation
pass is the outstanding work before this is ready.
main; it will be rebased before it goes up forreview.
Scope
219 files: 174 ADRs, 37
AGENTS.md, 2 triage-skill files, 3 analysis reports, andthree incidental fixes (a CODEOWNERS path, a doctoc exclusion, a dev-doc correction).
If this does go live, it will be split — this is far too large to land or
review as one change. Each area's criteria and ADRs would become its own PR,
reviewed individually by the people who own that area.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines