Skip to content

Commit 9c05e58

Browse files
authored
Add Copilot code review instructions for catching AI-slop PRs (#62442)
Add `.github/instructions/code-review.instructions.md` with review rules derived from patterns in recently rejected PRs. Covers architecture boundaries, database correctness, code quality, testing requirements, API/UI patterns, and signals of low-quality AI-generated contributions. Scoped to code review only (`excludeAgent: coding-agent`). Also excludes `.github/instructions/` from doctoc and insert-license pre-commit hooks since `.instructions.md` files require YAML frontmatter at the very start of the file.
1 parent 75444c2 commit 9c05e58

File tree

2 files changed

+71
-0
lines changed

2 files changed

+71
-0
lines changed
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
---
2+
applyTo: "**"
3+
excludeAgent: "coding-agent"
4+
---
5+
6+
# Airflow Code Review Instructions
7+
8+
Use these rules when reviewing pull requests to the Apache Airflow repository.
9+
10+
## Architecture Boundaries
11+
12+
- **Scheduler must never run user code.** It only processes serialized Dags. Flag any scheduler-path code that deserializes or executes Dag/task code.
13+
- **Workers must not access the metadata DB directly.** Task execution communicates with the API server through the Execution API (`/execution` endpoints) only.
14+
- **Dag Processor and Triggerer run user code in isolated processes.** Code in these components should maintain that isolation.
15+
- **Providers must not import core internals** like `SUPERVISOR_COMMS` or task-runner plumbing. Providers interact through the public SDK and execution API only.
16+
17+
## Database and Query Correctness
18+
19+
- **N+1 queries**: Flag SQLAlchemy queries that access relationships inside loops without `joinedload()` or `selectinload()`.
20+
- **`run_id` is only unique per Dag.** Queries that group, partition, or join on `run_id` alone (without `dag_id`) will collide across Dags. Always require `(dag_id, run_id)` together.
21+
- **Cross-database compatibility**: SQL changes must work on PostgreSQL, MySQL, and SQLite. Flag database-specific features (lateral joins, window functions) without cross-DB handling.
22+
- **Session discipline**: In `airflow-core`, functions receiving a `session` parameter must not call `session.commit()`. Use keyword-only `session` parameters.
23+
24+
## Code Quality Rules
25+
26+
- No `assert` in production code (stripped in optimized Python).
27+
- `time.monotonic()` for durations, not `time.time()`.
28+
- Imports at top of file. Valid exceptions: circular imports, lazy loading for worker isolation, `TYPE_CHECKING` blocks.
29+
- Guard heavy type-only imports (e.g., `kubernetes.client`) with `TYPE_CHECKING` in multi-process code paths.
30+
- Unbounded caches are bugs: all `@lru_cache` must have `maxsize`.
31+
- Resources (files, connections, sessions) must use context managers or `try/finally`.
32+
33+
## Testing Requirements
34+
35+
- New behavior requires tests covering success, failure, and edge cases.
36+
- Use pytest patterns, not `unittest.TestCase`.
37+
- Use `spec`/`autospec` when mocking.
38+
- Use `time_machine` for time-dependent tests.
39+
- Imports belong at the top of test files, not inside test functions.
40+
- Issue numbers do not belong in test docstrings.
41+
42+
## API Correctness
43+
44+
- `map_index` must be handled correctly for mapped tasks. Queries without `map_index` filtering may return arbitrary task instances.
45+
- Execution API changes must follow Cadwyn versioning (CalVer format).
46+
47+
## UI Code (React/TypeScript)
48+
49+
- Avoid `useState + useEffect` to sync derived state. Use nullish coalescing or nullable override patterns instead.
50+
- Extract shared logic into custom hooks rather than copy-pasting across components.
51+
52+
## AI-Generated Code Signals
53+
54+
Flag these patterns that indicate low-quality AI-generated contributions:
55+
56+
- **Fabricated diffs**: Changes to files or code paths that don't exist in the repository.
57+
- **Unrelated files included**: Changes to files that have nothing to do with the stated purpose of the PR.
58+
- **Description doesn't match code**: PR description describes something different from what the code actually does.
59+
- **No evidence of testing**: Claims of fixes without test evidence, or author admitting they cannot run the test suite.
60+
- **Over-engineered solutions**: Adding caching layers, complex locking, or benchmark scripts for problems that don't exist or are misunderstood.
61+
- **Narrating comments**: Comments that restate what the next line does (e.g., `# Add the item to the list` before `list.append(item)`).
62+
- **Empty PR descriptions**: PRs with just the template filled in and no actual description of the changes.
63+
64+
## Quality Signals to Check
65+
66+
The absence of these signals in a "fix" or "optimization" PR is itself a red flag:
67+
68+
- **Bug fixes need regression tests**: A test that fails without the fix and passes with it.
69+
- **Existing tests must still pass without modification**: If existing tests need changes to pass, the PR may introduce a behavioral regression.

.pre-commit-config.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ repos:
5656
exclude:
5757
(?x)
5858
.github/PULL_REQUEST_TEMPLATE\.md$|
59+
.github/instructions/|
5960
.github/skills/
6061
args:
6162
- "--maxlevel"
@@ -194,6 +195,7 @@ repos:
194195
exclude:
195196
(?x)
196197
^scripts/ci/license-templates/|
198+
^\.github/instructions/|
197199
^\.github/skills/airflow-translations/SKILL\.md$
198200
- id: insert-license
199201
name: Add license for all other files

0 commit comments

Comments
 (0)