From 9e0a117e0d10c48cb56e7a21f3a7396d996144ac Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 16 May 2026 16:41:00 -0700 Subject: [PATCH 1/3] Add AGENTS.md for LLM contributors (#12573) --- AGENTS.md | 475 ++++++++++++++++++++++++++++++++++++++ CHANGES/12573.contrib.rst | 7 + 2 files changed, 482 insertions(+) create mode 100644 AGENTS.md create mode 100644 CHANGES/12573.contrib.rst diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000000..0c31b77461f --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,475 @@ +# Notes for LLM contributors + +## Rule zero: prove it works before opening the PR + +**Your job is to deliver code that is proven to work.** If you +have not proven the change works, it is not time to open the PR +yet. "It compiles", "type checks pass", and "the diff looks +right" are not proof. Proof is: the relevant tests run locally +and pass, the new behaviour is exercised by a test you added or +extended, and any user-visible path you touched has been +executed end-to-end. For aiohttp specifically, that means the +suite passes under **both** the default C-extension build **and** +the pure-Python build (`AIOHTTP_NO_EXTENSIONS=1`); see the +[Tests](#tests) section for the exact commands. If you cannot +run the suite in your environment, say so explicitly in the PR +body rather than implying coverage you did not actually achieve. +Opening a PR that turns out not to work wastes the reviewer's +time and is the single fastest way to lose trust on this repo. + +The rest of this document covers how to dress up that proven +change for review. None of it matters if rule zero is not met. + +--- + +Read this before opening a pull request against `aio-libs/aiohttp`. +Agents keep getting the same things wrong in this repo, so the rules +below are not optional. If you are about to skip a section because it +sounds like boilerplate, that is the section to re-read. + +Human-facing contributor docs live under +[docs/contributing.rst](docs/contributing.rst), +[CONTRIBUTING.rst](CONTRIBUTING.rst), and +[CHANGES/README.rst](CHANGES/README.rst); this file is the short +orientation for agents. + +## What this project is + +`aiohttp` is the async HTTP client and server stack for the +`aio-libs` ecosystem. It is large, widely deployed, performance +sensitive, and ships **two parallel implementations of the hot +paths that must stay behaviourally identical**: a pure-Python +fallback and a set of C / Cython extensions. The C parser is +built on top of `llhttp`, which lives under `vendor/llhttp/` as +a **git submodule** pinned to a specific upstream commit; see +the [Submodules and llhttp](#submodules-and-llhttp) section +below. + +Useful entry points: + +| Path | What | +| ------------------------------------------ | -------------------------------------------------------------------- | +| `aiohttp/__init__.py` | public package surface and version | +| `aiohttp/client.py` | `ClientSession`, top-level client API | +| `aiohttp/client_reqrep.py` | `ClientRequest`, `ClientResponse` | +| `aiohttp/connector.py` | `TCPConnector`, connection pooling, DNS | +| `aiohttp/web.py` and `aiohttp/web_*.py` | server framework (app, request, response, runners, middlewares) | +| `aiohttp/http_parser.py` | pure-Python HTTP parser; dispatches to C parser when available | +| `aiohttp/_http_parser.pyx` | Cython HTTP parser bound to the `llhttp` submodule | +| `aiohttp/http_writer.py` | pure-Python HTTP writer; falls back from the Cython one | +| `aiohttp/_http_writer.pyx` | Cython HTTP writer | +| `aiohttp/_websocket/reader.py` | dispatcher that selects the Cython or pure-Python WS reader | +| `aiohttp/_websocket/reader_py.py` | pure-Python WebSocket reader | +| `aiohttp/_websocket/reader_c.py` | sibling source compiled by Cython; must match `reader_py.py` | +| `aiohttp/_websocket/mask.pyx` | Cython masking primitive (`websocket_mask`) | +| `aiohttp/_websocket/helpers.py` | shared WS helpers and the `websocket_mask` Python fallback | +| `aiohttp/hdrs.py` and `aiohttp/_headers.pxi` | known header constants; the `.pxi` is generated by `tools/gen.py` | +| `vendor/llhttp/` | git submodule pinned to a `nodejs/llhttp` commit (not a copy) | +| `tests/` | pytest suite, parametrised across both backends | +| `CHANGES/` | towncrier news fragments, one per PR | +| `docs/` | Sphinx docs source | + +`AIOHTTP_NO_EXTENSIONS=1` forces the pure-Python build at install +time; the default is the C extension. The dispatch flag at runtime is +`aiohttp.helpers.NO_EXTENSIONS`. + +## Pull request rules + +These are the rules agents most often violate. Treat them as mandatory. + +### 1. Use the shipped pull request template + +`aiohttp` ships its own template at +[`.github/PULL_REQUEST_TEMPLATE.md`](.github/PULL_REQUEST_TEMPLATE.md), +and `gh pr create` will load it automatically. **Use it as shipped.** +Do not invent your own `## What / ## Why / ## How / ## Testing` +layout; that is the marker that the PR was written by an agent +without reading the conventions. + +The template looks like this; fill it out verbatim: + +```markdown + + +## What do these changes do? + + + +## Are there changes in behavior for the user? + + + +## Is it a substantial burden for the maintainers to support this? + + + +## Related issue number + +Fixes #NNNN + + +## Checklist + +- [x] I think the code is well written +- [x] Unit tests for the changes exist +- [x] Documentation reflects the changes +- [x] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` +- [x] Add a new news fragment into the `CHANGES/` folder +``` + +Tick the boxes that actually apply. If a row does not apply (e.g. a +CI-only change with no tests), write `N/A` next to it rather than +silently leaving it blank. `CONTRIBUTORS.txt` is sorted alphabetically +by first name; insert your ` ` line in the right place +the first time you contribute, then leave it alone on follow-ups. + +### 2. Add a CHANGES fragment + +Every user-visible or contributor-visible PR needs a towncrier news +fragment in `CHANGES/`, named `..rst`. Categories +(defined in [CHANGES/README.rst](CHANGES/README.rst) and the +`[tool.towncrier]` section of [pyproject.toml](pyproject.toml)): + +| Category | When to use | +| -------------- | --------------------------------------------------------------- | +| `bugfix` | corrects undesired behaviour | +| `feature` | new public API or behaviour | +| `deprecation` | announces a future removal | +| `breaking` | removes or changes something public in a breaking way | +| `doc` | documentation structure or build process | +| `packaging` | downstream-visible packaging or build changes | +| `contrib` | contributor experience (CI, dev env, test invocation) | +| `misc` | does not fit any of the above | + +Conventions for the fragment body: + +- Use the past tense (`Fixed`, `Added`, `Bumped`), since it is read + as a "what changed since the previous release" digest. +- Use reStructuredText, not Markdown. +- Do not include the issue or PR number in the body; towncrier adds + it automatically from the filename. +- Sign with `` -- by :user:`github-handle` `` at the end. +- For multiple fragments in the same category, append a sequence + number: `1234.feature.rst`, `1234.feature.1.rst`. + +Example (`CHANGES/8074.bugfix.rst` style): + +```rst +Fixed an unhandled exception in the Python HTTP parser on header +lines starting with a colon -- by :user:`pajod`. +``` + +Pick the number for the fragment filename as follows: + +- **If the change has a linked issue, name the fragment after + the issue number** (e.g. `CHANGES/1234.bugfix.rst` for a fix + that closes `#1234`). The issue number is stable and known + before the PR is opened. +- **If there is no linked issue,** you have two options: + - Open the PR first, then add the fragment as a follow-up + commit on the same branch using the assigned PR number; or + - Guess the next PR number (scan + `gh pr list --repo aio-libs/aiohttp --state all --limit 5` + for the current top of the range), include the fragment in + your initial push, and rename in a follow-up commit if the + guess was off by the time the PR opened. +- **If both an issue and a PR number are in play and you want + both to resolve,** keep the issue-numbered file as the real + fragment and add a symlink at + `CHANGES/..rst` pointing to it, so + towncrier and the GitHub cross-reference both find the entry: + + ```bash + ln -s 1234.bugfix.rst CHANGES/1240.bugfix.rst + ``` + +### 3. Open the PR as a draft, and leave it that way + +Use `gh pr create --draft`. **Every LLM-authored submission +must be fully reviewed by a human before it is marked ready +out of draft, with no exceptions.** That review is the +responsibility of the person running the agent, not of the +project maintainers; do not shift the burden of reviewing LLM +work onto them. Maintainers do not look at drafts, so the +draft state is the agent's hand-off to the operator's review, +not a request for the project to review the code on the +operator's behalf. Do not mark the PR ready yourself, and do +not request reviewers from the agent session; the human who +reviewed the change and flipped it out of draft is the one who +routes it. + +### 4. Disclose the agent, do not advertise it + +Disclosure is required, advertising is not welcome. Put one +plain line at the bottom of the PR body naming the agent that +drafted the change, for example: + +``` +Drafted with ; reviewed by . +``` + +That single line is enough. Beyond that: + +- **No `Co-Authored-By:` trailers** for an LLM or any AI tool, in + commits or in the PR body. Attribution goes to the human who + reviewed the change. +- **Agent output goes in a footer below the PR summary, ideally + in a collapsed `
` block.** The aio-libs template + sections (What / Are there changes in behavior / etc.) come + first and read like a human wrote them. Anything the agent + wants to surface for reviewers (scan results, test logs, + branch hygiene notes, pipeline output) goes underneath that. + A collapsed `
` block at the very bottom is the + recommended shape; it keeps the summary readable while still + letting a curious reviewer expand the agent's work: + + ```markdown +
+ Agent run details (optional, for reviewers) + + Tests: + Lint: +
+ ``` + + What is not OK is mixing this content into the template + sections themselves, or pushing it above the human-readable + summary so reviewers have to scroll past it. The shape and + content of the footer is otherwise up to the agent. +- No emoji decoration (`🤖`, `✨`, `🚀`) in commit messages, PR + titles, PR bodies, or news fragments. Project style is plain prose. +- Commit messages and PR prose should read as if a human contributor + wrote them. Specifically: + - **No em-dashes (`—`)** and no dashes used as sentence separators + (`foo - bar`). Use a semicolon or a comma. This is the strongest + tell for AI-generated prose in this project, and reviewers do + read for it. + - No "Let me", "I'll", or first-person narration of what the agent + did. Describe the change, not the author. + - No filler sections ("Overview", "Summary of changes", "Key + takeaways") on top of the template. The template already has + the right sections. + +### 5. Keep the PR body short + +A couple of sentences per template section is plenty. If the change +is non-obvious, a short reproducer or a paragraph on root cause is +welcome. Long, multi-section essays with bolded sub-headings are +not the style here. + +### 6. Commit hygiene + +- One logical change per PR. If a refactor and a bugfix are bundled + together, split them. +- Pre-commit hooks (the changelog filename check, isort, black, + pyupgrade, flake8, yesqa, codespell, end-of-file-fixer, an + rst-linter, and others under + [`.pre-commit-config.yaml`](.pre-commit-config.yaml)) rewrite + files in place and may abort the commit; when that happens, + re-stage and commit again. Do not pass `--no-verify`. Mypy is + not in pre-commit; it runs via `make mypy` (and is included in + `make lint`). +- The repo does **not** use Conventional Commits as a CI gate. Recent + landed subjects are short imperative or descriptive prose (e.g. + `Fix digest authentication for URLs with reserved characters`, + `ci: report slowest benchmarks via --durations=30`, + `Bump pytest-codspeed from 5.0.1 to 5.0.2`). Match that style; do + not force `feat:` / `fix:` prefixes onto every commit. +- The default branch is `master`, not `main`. Open PRs against + `master`; backports to the active release branches (`3.13`, + `3.14`, etc.) are handled automatically by the `Patchback` + GitHub App once the merged PR carries a `backport-3.X` label. + Use `backport:skip` to opt a PR out of backporting. + +## Dual-backend discipline + +This is the single biggest source of broken aiohttp PRs from agents. + +aiohttp's hot paths live in both a Cython / C implementation and a +pure-Python fallback. When you change behaviour in one, check whether +the other needs the same change: + +- A bug fix in `aiohttp/http_parser.py` (pure-Python HTTP parser) + usually needs a matching fix in `aiohttp/_http_parser.pyx`. If + the bug is in the underlying C parser it lives upstream in + `nodejs/llhttp` (see [Submodules and llhttp](#submodules-and-llhttp)); + fix it there and bump the submodule pin in a separate PR. If + the C path is genuinely unaffected (different code path, + different invariant), say so explicitly in the "What do these + changes do?" section. +- A bug fix in `aiohttp/http_writer.py` usually needs a matching + fix in `aiohttp/_http_writer.pyx`. +- The WebSocket reader is split across + `aiohttp/_websocket/reader_py.py` (pure Python) and + `aiohttp/_websocket/reader_c.py` (the same source compiled by + Cython). **They must stay byte-for-byte equivalent in + behaviour**: any change to one must land in the other in the + same PR. The dispatcher is `aiohttp/_websocket/reader.py`. +- The WebSocket masking primitive has a Python fallback in + `aiohttp/_websocket/helpers.py` and a Cython implementation in + `aiohttp/_websocket/mask.pyx`; the same parity rule applies. +- A new public API must land in both backends with identical + signatures, identical behaviour, identical type hints, and + matching docstrings. +- Tests in `tests/` are exercised against both builds in CI (one + job runs with `AIOHTTP_NO_EXTENSIONS=1`), so a divergence will + surface as a failure on one of the two legs. Reproduce both + locally before opening the PR. + +If you can only fix one backend in scope, file a follow-up issue +and call it out in the PR body. Do not silently leave the +implementations divergent. + +## Tests + +Install dev deps, build the extensions, and run the suite: + +```bash +make .develop # installs deps, regenerates llhttp parser tables, builds Cython in place +make test # pytest -q against the built extension +``` + +`make vtest` runs verbose plus a `-X dev` pass, `make cov-dev` adds +coverage with an HTML report. + +To exercise both backends explicitly: + +```bash +# C extension (default) +pip install -e . +pytest -q + +# Pure Python +AIOHTTP_NO_EXTENSIONS=1 pip install -e . --force-reinstall --no-deps +pytest -q +``` + +`make lint` runs `pre-commit` across the tree plus `mypy`. + +CI runs the full matrix across the supported CPython versions +(3.10+), plus an `AIOHTTP_NO_EXTENSIONS=1` leg, a docs build, a +CodSpeed benchmark leg, and wheel builds for manylinux, musllinux, +macOS, and Windows. Do not regress the benchmarks without flagging +the trade-off in the PR body. + +## Cython and generated files + +`aiohttp/_http_parser.pyx`, `aiohttp/_http_writer.pyx`, and +`aiohttp/_websocket/mask.pyx` are compiled by Cython; the resulting +`*.c` and `*.so` files are build outputs and must not be committed. +`make cythonize` regenerates the `.c` siblings during development. + +`aiohttp/_headers.pxi` and `aiohttp/_find_header.c` are generated +from `aiohttp/hdrs.py` by `tools/gen.py`; regenerate by adding a +header in `hdrs.py` and running `make cythonize` (the rule runs +`tools/gen.py` for you). + +## Submodules and llhttp + +`vendor/llhttp/` is a **git submodule** that points at +[`nodejs/llhttp`](https://github.com/nodejs/llhttp); aiohttp's +checked-in tree only tracks the submodule sha (the current pin is +visible in `git submodule status`). Cloning aiohttp without +submodules will not work; `setup.py` exits with a hint to run +`git submodule update --init` if the checkout is missing. + +The C parser extension is built from three files inside the +submodule checkout, in this order: + +- `vendor/llhttp/build/c/llhttp.c`: the parser tables, + **generated from the TypeScript sources** by + `make generate-llhttp` (which runs `make -C vendor/llhttp generate` + after `npm ci`). This file is a build output; it is regenerated + locally and is not committed upstream. +- `vendor/llhttp/src/native/api.c` and `vendor/llhttp/src/native/http.c`: + tracked C sources inside the submodule, owned by the upstream + llhttp repo. + +What this means in practice: + +- **Do not edit anything under `vendor/llhttp/` by hand.** That is + upstream `nodejs/llhttp` code; fixes belong there, not in the + aiohttp tree. +- **Bumping the llhttp version is a submodule-pointer bump, not an + edit.** From inside `vendor/llhttp` run `git fetch && git checkout `, + then in the aiohttp root `git add vendor/llhttp` to record the + new pointer. Keep the bump in its own PR and call out which + upstream tag / sha you are moving to. +- `make generate-llhttp` only regenerates the build-time parser + tables for the currently pinned commit; it does **not** change + the pin, and there is nothing to commit afterwards in the + aiohttp tree. + +Files you should not edit by hand or commit alongside source +changes: + +- `aiohttp/*.c`, `aiohttp/*.html`, `aiohttp/*.so` +- `aiohttp/_websocket/*.c`, `aiohttp/_websocket/*.html`, + `aiohttp/_websocket/*.so` +- `*.py,cover` coverage artefacts +- `__pycache__/`, `.hash/`, build trees under `build/` and `dist/` + +## Documentation + +User-visible API changes need a docs update under `docs/` (the +relevant section of `docs/client_reference.rst` or +`docs/web_reference.rst` plus any narrative pages). The docstring +goes in the code; the prose context goes in the Sphinx sources. +`make doc` builds the docs locally; `make doc-spelling` runs the +spell-check leg that CI also runs. + +## Code style + +- `pyproject.toml` pins `requires-python = ">= 3.10"`. Match the + surrounding file's import and typing conventions; do not + introduce `from __future__ import annotations` to files that + do not already use it. +- Pre-commit runs isort, black, pyupgrade, flake8, yesqa, + codespell, end-of-file-fixer, the rst-linter, and the + changelog filename check, among others; mypy runs separately + via `make mypy`. Let the hooks rewrite files in place rather + than reformatting by hand. +- Do not add docstrings or comments that just restate the code. + Match the existing terse style in the surrounding module. + +## Things not to do + +- Do not open a PR for code you have not proven works (see + _Rule zero_ at the top of this file). Run the relevant tests + on **both** backends, cover the new behaviour with a test, + exercise the user-visible path end-to-end, and say so honestly + in the PR body if any of that was not possible in your + environment. +- Do not invent a `## What / ## Why / ## How / ## Testing` PR + body; use the shipped template at + `.github/PULL_REQUEST_TEMPLATE.md`. +- Do not skip the `CHANGES/` fragment "because the change is + small". Even a one-line bugfix needs one. +- Do not add `Co-Authored-By` trailers for LLM tools, in either + commits or the PR body. +- Do not mix agent-generated scan output, test summaries, or + pipeline reports into the template sections. Put them in a + collapsed `
` footer below the PR summary instead. +- Do not use em-dashes or sentence-separating dashes in PR prose + or commit messages. +- Do not edit files under `vendor/llhttp/` by hand; they belong + to the upstream `nodejs/llhttp` submodule. To bump the pinned + version, move the submodule pointer (see + [Submodules and llhttp](#submodules-and-llhttp)) and split that + bump into its own PR. +- Do not commit build artefacts (`*.c`, `*.so`, `*.html`, + `*.py,cover`, `__pycache__/`, `build/`, `dist/`) alongside + source changes. +- Do not change one backend without checking the other; see + "Dual-backend discipline" above. +- Do not open PRs against the release branches (`3.12`, `3.13`, + etc.); target `master` and let `patchback` handle the + backport after merge. +- Do not mark the PR ready for review yourself; that is the + call of the human running the agent, not the agent itself. + Maintainers do not look at drafts, but that does not mean + they should be doing your review; the operator is responsible + for reviewing the LLM-authored change before flipping the PR + out of draft. +- Do not request reviewers from the agent session; the human + who flips the PR out of draft will route it. diff --git a/CHANGES/12573.contrib.rst b/CHANGES/12573.contrib.rst new file mode 100644 index 00000000000..3e6680eff5a --- /dev/null +++ b/CHANGES/12573.contrib.rst @@ -0,0 +1,7 @@ +Added an ``AGENTS.md`` orientation file at the repository root, covering the +shipped pull request template, the ``CHANGES/`` news fragment conventions, +the draft-PR / human-review workflow, and the dual pure-Python and +C-extension parity rule (including the WebSocket +``reader_py.py`` / ``reader_c.py`` split and the vendored ``llhttp`` +sources), so LLM contributors land changes that match project style +-- by :user:`bdraco`. From 5edd76f04c938e7de93b870ac7aa57a8accd87dc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 16 May 2026 17:57:15 -0700 Subject: [PATCH 2/3] Parallelize Cython extension compilation (#12576) --- CHANGES/12576.packaging.rst | 4 ++++ setup.py | 14 +++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 CHANGES/12576.packaging.rst diff --git a/CHANGES/12576.packaging.rst b/CHANGES/12576.packaging.rst new file mode 100644 index 00000000000..dc748bfe726 --- /dev/null +++ b/CHANGES/12576.packaging.rst @@ -0,0 +1,4 @@ +Parallelized the Cython extension compilation by defaulting +``build_ext.parallel`` to ``os.cpu_count()``, so each module's +``gcc`` invocation now runs concurrently instead of one at a time +-- by :user:`bdraco`. diff --git a/setup.py b/setup.py index 72fd074b66f..0933d969b00 100644 --- a/setup.py +++ b/setup.py @@ -3,6 +3,7 @@ import sys from setuptools import Extension, setup +from setuptools.command.build_ext import build_ext if sys.version_info < (3, 10): raise RuntimeError("aiohttp 4.x requires Python 3.10+") @@ -87,8 +88,19 @@ ] +class ParallelBuildExt(build_ext): + def build_extensions(self) -> None: + if self.parallel is None: + self.parallel = os.cpu_count() or 1 + super().build_extensions() + + build_type = "Pure" if NO_EXTENSIONS else "Accelerated" -setup_kwargs = {} if NO_EXTENSIONS else {"ext_modules": extensions} +setup_kwargs = ( + {} + if NO_EXTENSIONS + else {"ext_modules": extensions, "cmdclass": {"build_ext": ParallelBuildExt}} +) print("*********************", file=sys.stderr) print("* {build_type} build *".format_map(locals()), file=sys.stderr) From ac79b7b72fc62e2cbc7e245bbdb29b3f8bdf08fb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 16 May 2026 21:40:27 -0700 Subject: [PATCH 3/3] Document agent pitfalls in AGENTS.md: test coverage and docs spell check (#12580) --- AGENTS.md | 78 +++++++++++++++++++++++++++++++++++++- CHANGES/12580.contrib.rst | 8 ++++ docs/spelling_wordlist.txt | 1 + 3 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 CHANGES/12580.contrib.rst diff --git a/AGENTS.md b/AGENTS.md index 0c31b77461f..2eec53c47f1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -257,7 +257,32 @@ is non-obvious, a short reproducer or a paragraph on root cause is welcome. Long, multi-section essays with bolded sub-headings are not the style here. -### 6. Commit hygiene +### 6. Run the docs spell check before pushing + +The `lint` CI job builds the docs with `sphinxcontrib.spelling` +under `make doc-spelling`, which is invoked with `-W +--keep-going` so any unknown word is a hard failure. The spell +checker reads every `CHANGES/*.rst` fragment as part of the +build, so a technical word in your news fragment (`codecov`, +`monkeypatch`, `parametrize`, `repr`, and so on) that is +not in +[`docs/spelling_wordlist.txt`](docs/spelling_wordlist.txt) +will fail `make doc-spelling` and burn a CI run before a human +even sees the PR. + +Before pushing: + +```bash +make doc-spelling +``` + +If it flags a word you actually meant to use, add it to +`docs/spelling_wordlist.txt` (one word per line, roughly +alphabetical) in the same commit as the fragment. If it flags +a typo, fix the typo. Do not paper over real misspellings by +adding them to the wordlist. + +### 7. Commit hygiene - One logical change per PR. If a refactor and a bugfix are bundled together, split them. @@ -352,6 +377,48 @@ CodSpeed benchmark leg, and wheel builds for manylinux, musllinux, macOS, and Windows. Do not regress the benchmarks without flagging the trade-off in the PR body. +### Every line in a test must be covered + +Coverage in this repo tracks both `aiohttp/` and `tests/`; the +CI test jobs run pytest with `--cov=aiohttp/ --cov=tests/` and +the cython-coverage job does the same, so uncovered lines in +`tests/` show up in the codecov patch report on every PR. +Reviewers do look at that report. This catches a class of +mistake agents make all the time: a defensive +`raise RuntimeError("must not be called")` inside a +monkeypatched stub the happy path never invokes, a cleanup +branch behind `if had_own_attr:` that the test never enters, an +`else` arm guarding a condition that is always true under the +fixture. From the perspective of the unit suite all of those +lines are dead code, and the codecov report flags them the same +as dead code in `aiohttp/`. + +Design tests so every line runs: + +- Drive the fixture deterministically so both arms of any + conditional are hit, or drop the conditional entirely and + assert the single shape you actually set up. +- Do not add `raise TypeError("must not be invoked")` guards + inside stubs the test installs; if the stub is never meant to + fire, either omit it or assert at the call site that it did + not. An unreachable `raise` is the most common form of this + failure. +- Cleanup branches that only run when setup took a particular + shape (`if had_own_attr: ...` style restores) need a second + test, or a parametrize, that exercises the other shape. If + you cannot justify the second case, unconditionally restore + instead. +- Prefer `monkeypatch` (which auto-reverts) over hand-rolled + save/restore blocks; the auto-revert path has no untaken + branch for coverage to flag. + +See [aio-libs/yarl#1687](https://github.com/aio-libs/yarl/pull/1687) +for the canonical example: a test added an unreachable `raise` +inside a patched `__getstate__` and a conditional restore of the +original attribute, both of which the codecov report rejected as +uncovered. The same pattern recurs in aiohttp test PRs and the +same review feedback applies. + ## Cython and generated files `aiohttp/_http_parser.pyx`, `aiohttp/_http_writer.pyx`, and @@ -443,6 +510,10 @@ spell-check leg that CI also runs. - Do not invent a `## What / ## Why / ## How / ## Testing` PR body; use the shipped template at `.github/PULL_REQUEST_TEMPLATE.md`. +- Do not push without running `make doc-spelling` first if you + edited any `.rst` file (including a `CHANGES/` fragment). The + docs build fails on unknown words and burns a CI run; see + _Run the docs spell check before pushing_ above. - Do not skip the `CHANGES/` fragment "because the change is small". Even a one-line bugfix needs one. - Do not add `Co-Authored-By` trailers for LLM tools, in either @@ -462,6 +533,11 @@ spell-check leg that CI also runs. source changes. - Do not change one backend without checking the other; see "Dual-backend discipline" above. +- Do not leave unreachable lines in tests (defensive `raise` + inside a stub the suite never invokes, cleanup branches that + only run for a setup shape the test does not exercise). Tests + are part of the coverage report; see _Every line in a test + must be covered_ above. - Do not open PRs against the release branches (`3.12`, `3.13`, etc.); target `master` and let `patchback` handle the backport after merge. diff --git a/CHANGES/12580.contrib.rst b/CHANGES/12580.contrib.rst new file mode 100644 index 00000000000..fd30647dbd2 --- /dev/null +++ b/CHANGES/12580.contrib.rst @@ -0,0 +1,8 @@ +Documented in :file:`AGENTS.md` that the coverage report covers +``tests/`` as well as ``aiohttp/`` (so unreachable defensive +``raise`` guards in patched stubs and one-sided cleanup branches +in tests will surface as uncovered on the codecov patch report), +and that the docs spell check (``make doc-spelling``) is a hard +CI gate that reads every ``CHANGES/*.rst`` fragment and should be +run locally before pushing +-- by :user:`bdraco`. diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 43ff5c75748..c81e125a3d4 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -72,6 +72,7 @@ CIMultiDict ClientSession cls cmd +codecov codebase codec Codings