Skip to content

cleanup: remove dead aiohttp vcr compat shim (#2140) - #2166

Closed
feiiiiii5 wants to merge 1 commit into
Open-Source-Legal:mainfrom
feiiiiii5:cleanup/remove-aiohttp-vcr-shim-2140
Closed

cleanup: remove dead aiohttp vcr compat shim (#2140)#2166
feiiiiii5 wants to merge 1 commit into
Open-Source-Legal:mainfrom
feiiiiii5:cleanup/remove-aiohttp-vcr-shim-2140

Conversation

@feiiiiii5

Copy link
Copy Markdown

Summary

Resolves #2140.

ensure_aiohttp_vcr_compat() was a transitional shim that monkey-patched
aiohttp.streams.AsyncStreamReaderMixin back into existence at import
time, so that vcrpy 8.1.1's aiohttp stub could subclass it. The shim is
no longer needed:

  • vcrpy 8.2.0+ dropped the broken mixin reference
    (kevin1024/vcrpy#996)
  • requirements/local.txt already pins vcrpy==8.3.0

What's removed

  • opencontractserver/utils/vcr_replay.py — the ensure_aiohttp_vcr_compat()
    function and its call site inside maybe_vcr_cassette().
  • conftest.py — the import + invocation of the shim at session start
    (and the 10-line comment block that justified it).
  • opencontractserver/tests/test_vcr_replay.py — the
    EnsureAiohttpVcrCompatTests class (3 tests) that asserted the shim
    worked. The class was testing the shim, not vcrpy itself.
  • requirements/local.txt — the eight-line comment block above the
    vcrpy pin explaining when to bump / delete the shim. Replaced with a
    one-liner that points at issue Investigate Possibly Deprecated VCR.py Helpers #2140 for future readers.

Net diff: −137 / +9 across 5 files.

Verification

  • mypy --config-file mypy.ini opencontractserver/utils/vcr_replay.py opencontractserver/tests/test_vcr_replay.py conftest.py
    Success: no issues found in 3 source files
  • python -m unittest opencontractserver.tests.test_vcr_replay
    Ran 35 tests in 0.6s — all NormalizeBodyTests,
    MatchLlmBodyTests, MaybeVcrCassetteTests, LlmHostsTests pass
    with the shim removed and the test class deleted.
  • python scripts/collate_changelog.py --checkOK: 252 changelog fragment(s) valid (the new fragment included).

Why this is safe

The shim existed solely to paper over vcrpy 8.1.1's broken
AsyncStreamReaderMixin reference. With the pin at 8.3.0, importing
vcr.stubs.aiohttp_stubs (which vcr.VCR().use_cassette(...) does
lazily) no longer raises AttributeError. I confirmed this by
comparing the stub file across vcrpy 8.1.1 vs 8.2.0 vs 8.3.0 — the
offending class MockStream(asyncio.StreamReader, streams.AsyncStreamReaderMixin)
became class MockStream(asyncio.StreamReader) in 8.2.0+.

The shim's only consumers were:

  1. conftest.py at import time (covered by the test suite).
  2. opencontractserver/utils/vcr_replay.py::maybe_vcr_cassette — the
    live E2E record/replay harness's belt-and-braces call site, which
    ran the shim before importing vcr. With 8.3.0 the call is a
    no-op, so removing it changes nothing observable.

Checklist

  • changelog.d/2140-remove-aiohttp-vcr-shim.removed.md added
  • No Claude / Claude Code attribution in commit message, PR body,
    or comments (per CLAUDE.md)
  • Branch cleanup/remove-aiohttp-vcr-shim-2140 based on
    upstream/main at 6d8f21ae0

vcrpy 8.2.0+ restored aiohttp 3.14 compatibility — the broken
`AsyncStreamReaderMixin` reference was dropped in kevin1024/vcrpy#996 —
and `requirements/local.txt` already pins `vcrpy==8.3.0`. The compat
shim in `opencontractserver/utils/vcr_replay.py`, its call site in
`conftest.py`, the `EnsureAiohttpVcrCompatTests` test class, and the
eight-line comment block above the vcrpy pin were therefore dead code.

Resolves Open-Source-Legal#2140.
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@feiiiiii5

Copy link
Copy Markdown
Author

Quick note on the failing claude-review check: it is not a PR issue. The job fails before any review logic runs, at the OIDC token fetch step:

Failed to get OIDC token: Unable to get ACTIONS_ID_TOKEN_REQUEST_URL env variable
Attempt 1 failed: Could not fetch an OIDC token. Did you remember to add `id-token: write` to your workflow permissions?

Fork PRs run with a restricted default GITHUB_TOKEN and do not expose ACTIONS_ID_TOKEN_REQUEST_URL unless id-token: write is explicitly granted to pull_request_target events from forks. The claude-review job (.github/workflows/*) appears to require an OIDC token to mint an Anthropic API token, which fork PRs cannot provide by default — same class of issue as documented for actions/checkout-style workflows that need OIDC.

All functional checks are green on this PR:

Check Status
pytest success
linter success
redis-integration success
Extract pipeline (PDF upload → run → CSV) success
WebSocket auth handshake success
codecov/patch (Frontend/Backend) success
changes success

The claude-review failure is a repo workflow config limitation for fork PRs, not something this PR can address. If the maintainers want claude-review to run on fork PRs, the workflow would need either id-token: write permission + pull_request_target trigger, or a workflow_run-based dispatcher. Happy to open a separate PR for that if useful — but it is out of scope for #2140.

@feiiiiii5

Copy link
Copy Markdown
Author

Closing for the same reason as #2165 — consolidating my open PRs. The dead-shim cleanup is preserved on my fork branch and can be reopened anytime. Thanks!

@feiiiiii5 feiiiiii5 closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate Possibly Deprecated VCR.py Helpers

1 participant