Lift aiohttp<3.14 cap with a vcrpy aiohttp-stub compat shim (closes #1920)

[JSv4]

Closes #1920.

Context

requirements/base.txt carried a temporary aiohttp>=3.13,<3.14 cap (added in #1914). aiohttp is an unpinned transitive dep (via llama-index-core), and aiohttp 3.14 removed aiohttp.streams.AsyncStreamReaderMixin, which vcrpy 8.1.1's aiohttp stub (vcr/stubs/aiohttp_stubs.py:18) subclasses:

class MockStream(asyncio.StreamReader, streams.AsyncStreamReaderMixin):

The blocker on the issue's literal checklist

The issue's action items are gated on "when vcrpy is bumped to a release that supports the aiohttp 3.14 stream API." No such release exists yet — vcrpy 8.1.1 (Jan 2026) is still the latest, the incompatibility is tracked in kevin1024/vcrpy#995, and the proposed fix (PR #996) is unreleased. So "bump vcrpy" is not currently possible, and pinning to an unmerged upstream PR would violate the repo's clean-pinning conventions.

Approach — surgical, removable shim

This codebase only records/replays httpx (LLM provider) cassettes; the aiohttp stub is pulled in incidentally because aiohttp is importable. vcrpy imports that stub lazily when a cassette is entered (vcr/patch.py builds its patchers), so under aiohttp ≥3.14 every VCR-using test errors with AttributeError. Because MockStream is never instantiated here, restoring the removed symbol as an empty mixin is sufficient.

• requirements/base.txt — removed the aiohttp<3.14 cap (back to unpinned-transitive; floats to 3.14+).
• opencontractserver/utils/vcr_replay.py — new ensure_aiohttp_vcr_compat(): idempotent, restores aiohttp.streams.AsyncStreamReaderMixin as an empty mixin when missing; no-op under aiohttp <3.14 (real mixin left untouched). Also called inside maybe_vcr_cassette() for the non-pytest E2E harness.
• conftest.py — applies the shim at conftest import, before any test runs.
• requirements/local.txt — vcrpy stays pinned at 8.1.1 with a breadcrumb pointing at the shim and Add Redis integration tests and CI workflow #996.
• Regression tests — opencontractserver/tests/test_vcr_replay.py::EnsureAiohttpVcrCompatTests.

Every vcr entry point is covered: the 7 integration test files (via conftest) and both runtime call sites (data_extract_tasks.py, unified_agent_conversation.py) route through maybe_vcr_cassette().

Verification

Reproduced and verified in throwaway venvs against the exact dependency combos:

Scenario	Result
aiohttp 3.14.0 + vcrpy 8.1.1, no shim	use_cassette() raises AttributeError: module 'aiohttp.streams' has no attribute 'AsyncStreamReaderMixin' (bug reproduced)
aiohttp 3.14.0 + vcrpy 8.1.1, with shim	maybe_vcr_cassette() works end-to-end; regression tests pass
aiohttp 3.13.5 + vcrpy 8.1.1	shim is a no-op (real mixin preserved); vcr works natively; tests pass

black, flake8, and the changelog --check all pass.

Follow-up

Delete the shim and bump vcrpy once a release ships the vcrpy#996 fix. The breadcrumbs in requirements/local.txt and the shim docstring point there.

[claude]

Code Review

Overview

This PR lifts the temporary aiohttp<3.14 cap in requirements/base.txt by introducing a small, idempotent import-time shim that restores aiohttp.streams.AsyncStreamReaderMixin — a symbol removed in aiohttp 3.14 — so vcrpy 8.1.1's aiohttp stub can evaluate under newer aiohttp versions. The approach is correct, minimal, and well-documented.

---

What's Good

• Minimal footprint: The shim only patches the module symbol when it is missing; it is a no-op under aiohttp < 3.14. The early-return guard means the class literal is only constructed once, not on every call.
• Both entry points covered: The shim is applied in conftest.py (before any pytest test module does a top-level import vcr) and inside maybe_vcr_cassette() (for the non-pytest E2E harness). That covers every VCR entry point in the codebase.
• Clear removal instructions: The shim docstring, the requirements/local.txt comment, and the changelog fragment all point to kevin1024/vcrpy#996 and say "delete when vcrpy ships a fix." Future maintainers won't have to guess.
• Test coverage: The three regression tests cover the symbol-present guarantee, idempotency, and the real integration contract (cassette entry without AttributeError). os and tempfile were already imported in the test file, so no missing imports.
• Changelog fragment: Correct filename format (1920-aiohttp-cap.changed.md), content matches the fragment convention.

---

Issues

CLAUDE.md Violation — Claude Code attribution in PR body

The PR description ends with:

_Generated by [Claude Code](https://claude.ai/code/session_01J7uRJoXbutgycPYccMsuwa)_

CLAUDE.md is explicit:

Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts. This includes Co-Authored-By lines, "Generated by Claude", etc.

Please edit the PR description to remove this line before merging.

---

Minor Observations (non-blocking)

• Test isolation nuance: Because conftest.py applies the shim before any test runs, test_mixin_symbol_present_after_shim always sees the symbol already present and only exercises the idempotent/no-op path in the full suite. The "add missing symbol" code path is only independently exercised in an isolated environment. This is an inherent constraint of the conftest design and test_vcr_cassette_entry_works provides the meaningful end-to-end coverage, so it is acceptable as-is.
• Class defined inside function: The AsyncStreamReaderMixin class is defined inside the function body, past the early-return guard, so it is only created when actually needed. No concern here — just noting it is intentional.

---

Summary

Technically sound and well-executed. The one required change is removing the Claude Code attribution from the PR body per project convention. Code itself is ready to merge once that is addressed.

[claude]

Code Review

Overview

This PR lifts the temporary aiohttp<3.14 cap by introducing a surgical import-time shim (ensure_aiohttp_vcr_compat()) that restores aiohttp.streams.AsyncStreamReaderMixin when missing. The root cause is clear: vcrpy 8.1.1 subclasses this symbol in its aiohttp stub at class-definition time, aiohttp 3.14 removed it, and vcrpy has no released fix yet. The approach is minimal, well-documented, and explicitly scoped for removal.

---

Code Quality

opencontractserver/utils/vcr_replay.py — The shim is clean:

• Correct hasattr guard for idempotency.
• setattr(streams, "AsyncStreamReaderMixin", AsyncStreamReaderMixin) is the right call (avoids mypy complaints on a removed attribute). Comment explaining the choice is appreciated.
• The class-inside-function pattern means a new class object would be created on each call to ensure_aiohttp_vcr_compat() if the guard didn't short-circuit — but since the guard does short-circuit after the first write, this is fine in practice.
• Docstring is thorough without being verbose.

conftest.py — Placing the shim here, before any test module is imported, is exactly right. The comment block explains the reasoning clearly.

maybe_vcr_cassette() — Good defensive placement for the non-pytest E2E harness that runs outside conftest scope.

---

Test Coverage

The EnsureAiohttpVcrCompatTests suite covers the right surface area:

• test_mixin_symbol_present_after_shim: Validates presence post-shim; correctly checks the real mixin is untouched under aiohttp < 3.14.
• test_shim_is_idempotent: Verifies the same object is returned on repeated calls — important since conftest and maybe_vcr_cassette() both invoke the shim.
• test_vcr_cassette_entry_works: The critical regression test — actually enters a VCR cassette rather than just importing vcr. This is the right level of test because import vcr alone doesn't trigger the stub import (as the comment notes).

One minor observation: test_mixin_symbol_present_after_shim doesn't restore streams.AsyncStreamReaderMixin to its original value in teardown. This is benign since conftest already applied the shim before tests run, but a self.addCleanup / try/finally would make the test properly self-contained. Non-blocking.

---

Potential Issues

1. Double blank line in requirements/base.txt (cosmetic): After removing the aiohttp line, there are now two consecutive blank lines before # Django. Worth a one-line cleanup:

   -brotli>=1.2.0  # ...
   -
   -
   -# Django
   +brotli>=1.2.0  # ...
   +
   +# Django

2. PR body credits Claude Code (_Generated by [Claude Code]..._): CLAUDE.md explicitly states: "Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts." Please remove that attribution line from the PR description.

3. Thread safety (non-issue in practice): setattr on a module object is not thread-safe, but since this runs at conftest import time—before any workers start—this is not a real concern. Worth a single-line note if you want to be explicit, but not required.

---

Security

No concerns. The shim only adds a missing name to an existing module object at import time and cannot affect runtime behavior for the httpx cassettes this project actually uses.

---

Summary

Verdict: Approve with minor notes. The approach is correct and the tradeoff is well-reasoned — a small, explicitly temporary shim is better than keeping a transitive dependency pinned to an older minor version. The breadcrumbs in requirements/local.txt and the shim docstring pointing to vcrpy#996 make future cleanup straightforward.

Blocking: Remove the Claude Code attribution from the PR description (CLAUDE.md requirement).
Non-blocking: Double blank line in requirements/base.txt; optional teardown in test_mixin_symbol_present_after_shim.

[claude]

Code Review

Overview

This PR lifts the temporary aiohttp<3.14 cap from requirements/base.txt by introducing a small, idempotent import-time shim that restores aiohttp.streams.AsyncStreamReaderMixin as an empty class when running under aiohttp >=3.14. The root cause is that vcrpy 8.1.1 subclasses that symbol in its aiohttp stub, and aiohttp 3.14 removed it — but since this codebase only replays httpx (LLM) cassettes, the aiohttp stub's MockStream is never instantiated, so an empty stand-in is sufficient.

---

What Works Well

• Shim design is surgical and minimal. The function is idempotent (early-return if the symbol is already present), is a no-op under aiohttp <3.14 (real mixin left untouched), and explicitly documents its own deletion criteria. The setattr/getattr pattern to dodge mypy's attr-defined check is the right call given that the attribute no longer exists in aiohttp 3.14's type stubs.

• All VCR entry points are covered. conftest.py applies the shim before any test module is imported (the critical ordering requirement — import vcr alone doesn't trigger the stub, but entering a cassette does), and maybe_vcr_cassette() covers the non-pytest E2E harness. This is thorough.

• Regression tests are well-constructed.

  • test_mixin_symbol_present_after_shim uses self.addCleanup to restore the module state, which is the right way to handle test isolation when mutating a third-party module.
  • test_vcr_cassette_entry_works exercises the real failure path (entering a cassette, not just importing vcr), which is where the AttributeError actually fires. This is the most valuable test of the three.
  • test_shim_is_idempotent correctly validates the double-call scenario (conftest + maybe_vcr_cassette).

• Documentation trail is complete. The shim docstring, the comment in requirements/local.txt, and the changelog fragment all cross-reference Lift the aiohttp<3.14 cap once vcrpy supports the 3.14 stream API #1920 and vcrpy#996. Future maintainers have a clear "delete when vcrpy ships X" checklist.

• Changelog fragment follows project conventions (correct filename type .changed.md, no section headers in body).

---

Minor Observations

1. test_shim_is_idempotent does not restore state. Unlike test_mixin_symbol_present_after_shim, this test calls ensure_aiohttp_vcr_compat() without a cleanup. Under aiohttp >=3.14 it leaves the shim-added symbol in place for any test that runs after it. In practice this is harmless (conftest already set it, and every VCR-using test needs it), but it's worth noting in case test ordering changes. Consider adding the same addCleanup pattern if strict isolation is a goal.

2. Empty-mixin thoroughness in test_mixin_symbol_present_after_shim. Under aiohttp >=3.14 (where had_real_symbol is False), the test confirms the symbol exists but doesn't assert it's specifically the shim class. Not a blocking issue since test_vcr_cassette_entry_works covers the functional contract, but a small additional assertion (e.g. checking no unexpected methods) would make the unit test more precise.

3. ModuleNotFoundError vs ImportError. The except ModuleNotFoundError in ensure_aiohttp_vcr_compat correctly handles the "aiohttp not installed" case. No change needed — just confirming this is intentional rather than an oversight.

4. Future aiohttp cassette work. The PR correctly notes that MockStream is never instantiated here. The shim docstring captures this assumption clearly. If aiohttp-based cassette recording is ever added, the shim's empty mixin would silently give MockStream a broken base class — worth a note in any future aiohttp cassette work, but not an issue today.

---

Security / Performance

No security concerns. The shim only adds a name to a module's namespace; it does not alter existing symbols or expose new attack surface. The hasattr guard ensures zero overhead on aiohttp <3.14.

---

Summary

This is a clean, well-reasoned workaround for a genuine upstream blocker. The implementation is correct, all call sites are covered, and the breadcrumbs for eventual cleanup are in place. The observations above are non-blocking. Good to merge.