Skip to content

Unbreak the linter job: pin flake8-isort/isort, and sync the mypy hook's stub pins - #2281

Merged
JSv4 merged 3 commits into
mainfrom
claude/pr-batch-ci-review-uztna9
Aug 30, 2026
Merged

Unbreak the linter job: pin flake8-isort/isort, and sync the mypy hook's stub pins#2281
JSv4 merged 3 commits into
mainfrom
claude/pr-batch-ci-review-uztna9

Conversation

@JSv4

@JSv4 JSv4 commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two .pre-commit-config.yaml pin problems found while getting #2257, #2260, #2264 and #2265 to green. The first is breaking main right now; the second is a trap the pending stub bumps walk into.

1. The linter job is red on main and on every open PR, with no commit responsible.

It fails with 10 I001/I005 findings in opencontractserver/llms/agents/pydantic_ai_agents.py and opencontractserver/utils/compact_pawls.py — files none of those PRs touch. The failure reproduces identically on a clean origin/main checkout.

Cause: the flake8 hook declared additional_dependencies: [flake8-isort] with no version. pre-commit re-resolves additional_dependencies from scratch every time it rebuilds a hook env, so flake8-isort — which re-implements the isort check inside flake8 using whatever isort it resolves — drifted up to isort 9.0.1, while the standalone isort hook stayed on the 6.0.1 its rev pins. isort 9 and isort 6 disagree about repeated from X import (...) statements, which is exactly the shape isort 6 emits for aliased imports. So the isort hook kept writing a layout the flake8 hook rejected. Nothing in the tree had to change for CI to go red.

The mypy hook immediately below already documents this exact failure mode — "pre-commit autoupdate only bumps rev; additional_dependencies are resolved fresh every time the hook env is (re)built, so anything less than == can drift" — the flake8 hook just never followed it.

2. The mypy hook's stub pins had drifted from requirements/local.txt, and closing the gap is not a no-op.

That hook's own comment says its ==-pinned stubs MUST match requirements/base.txt + requirements/local.txt. They didn't (django-stubs 6.0.6 vs 6.0.7, djangorestframework-stubs 3.17.0 vs 3.17.1), and #2265 / #2260 widen the gap to 6.1.0 / 3.18.0. Because CI type-checks with the hook's stubs, those bumps land green while leaving the dev/test image type-checking against something else.

Syncing the pins surfaces 7 previously-invisible errors from django-stubs 6.1.0 — the "Explicit through default manager" change the reviewer on #2265 flagged as worth checking. All 7 are typing-only; no runtime behavior changes.

Changes

  • .pre-commit-config.yaml — flake8 hook: additional_dependencies now ==-pins flake8-isort==7.0.0 and isort==6.0.1, with a comment noting isort here must track the isort hook's rev.
  • .pre-commit-config.yaml — mypy hook: django-stubs 6.0.6 → 6.1.0, djangorestframework-stubs 3.17.0 → 3.18.0.
  • opencontractserver/shared/QuerySets.py (6 errors) — six guardian-permission id lists are built as a lazy values_list queryset inside a try and fall back to [] in the matching except LookupError. 6.1.0 types values_list(..., flat=True) as QuerySet[Model, int], which no longer unifies with list[Never]. Each variable now carries an explicit Iterable[Any] declaration — they are only ever consumed by an __in lookup, so that is the accurate common type.
  • opencontractserver/tests/test_corpus_canonical_caml_migration.py:161 (1 error) — the test asserts apps.get_model("corpuses", "CorpusDescriptionRevision") raises LookupError, i.e. the lazy reference is unresolvable on purpose. 6.1.0's plugin resolves get_model() string pairs statically and errors on a miss, so this is a false positive. Silenced narrowly with # type: ignore[misc] plus a comment; the test's logic and assertions are unchanged.
  • changelog.d/flake8-isort-pin.fixed.md, changelog.d/mypy-hook-stub-parity.changed.md.

Test plan

pre-commit run --all-files
  • On this branch: all hooks pass, including flake8 and mypy.
  • pre-commit run flake8 --all-files on a clean origin/main checkout reproduces all 10 I001/I005 findings before the pin, and passes after it.
  • Reverting only the two stub pins (keeping the code fixes) is green; reverting only the code fixes (keeping the stub pins) reproduces exactly the 7 errors listed above — so the two halves of change Bump traefik from v2.8.7 to v2.9.1 in /compose/production/traefik #2 are matched.
  • Verified the isort skew directly: the flake8 hook env resolved isort 9.0.1, the isort hook env 6.0.1.

Relationship to the open dependency bumps

Ordering does not matter for CI, but the end state is only self-consistent once all three land:

Checklist

  • Tests pass locally for any code this PR touches
  • pre-commit run --all-files passes (black, isort, flake8, mypy)
  • TypeScript compiles cleanly — n/a, no frontend code changed
  • A changelog fragment was added under changelog.d/
  • Any new dependency was checked — no new dependency; two existing hook dependencies were pinned to versions already in requirements/local.txt

🤖 Generated with Claude Code

https://claude.ai/code/session_01Q3mE12T4oCZNyorWYJKF5R


Generated by Claude Code

claude added 2 commits August 30, 2026 03:31
…urfaces

.pre-commit-config.yaml's mypy hook documents that its `==`-pinned stub
dependencies MUST match requirements/base.txt + requirements/local.txt, because
the hook is what CI actually type-checks with. They had drifted
(django-stubs 6.0.6 vs 6.0.7, djangorestframework-stubs 3.17.0 vs 3.17.1), and
the pending Dependabot bumps #2265 (django-stubs 6.1.0) and #2260
(djangorestframework-stubs 3.18.0) widen the gap further.

Bumping the hook to those versions is not a no-op: django-stubs 6.1.0 surfaces
7 previously-invisible type errors. All 7 are typing-only; no runtime behavior
changes.

  QuerySets.py x6 - six guardian-permission id lists are built as a lazy
  `values_list` queryset inside a `try` and fall back to `[]` in the matching
  `except LookupError`. 6.1.0 types `values_list(..., flat=True)` as
  `QuerySet[Model, int]`, which no longer unifies with `list[Never]`. Each
  variable now carries an explicit `Iterable[Any]` declaration; they are only
  ever consumed by an `__in` lookup, so that is the accurate common type.

  test_corpus_canonical_caml_migration.py x1 - the test asserts
  `apps.get_model("corpuses", "CorpusDescriptionRevision")` raises LookupError,
  i.e. the lazy reference is unresolvable on purpose. 6.1.0's plugin resolves
  get_model() string pairs statically and errors on a miss, so this is a false
  positive; silenced narrowly with `# type: ignore[misc]`.

Verified with `pre-commit run --all-files`: mypy passes with the bumped stubs
(it fails with 7 errors without these fixes). black/isort/pyupgrade/yaml/
changelog hooks all pass. flake8 reports 10 pre-existing I001/I005 findings in
pydantic_ai_agents.py and compact_pawls.py that reproduce identically on a clean
origin/main checkout and are untouched by this change.
The linter job is failing on main and on every open PR with 10 I001/I005
findings in two files nobody edited (llms/agents/pydantic_ai_agents.py,
utils/compact_pawls.py). No commit caused it.

The flake8 hook declared `additional_dependencies: [flake8-isort]` with no
version. pre-commit re-resolves additional_dependencies from scratch whenever it
rebuilds a hook env, so flake8-isort — which re-implements the isort check
inside flake8 using whatever isort it resolves — drifted to isort 9.0.1, while
the standalone `isort` hook stayed on the 6.0.1 its `rev` pins. isort 9 and
isort 6 disagree about repeated `from X import (...)` statements, which is
exactly the shape isort 6 emits for aliased imports. So the isort hook wrote a
layout the flake8 hook rejected, on an unchanged tree.

The mypy hook right below already documents this exact failure mode ('anything
less than == can drift'); the flake8 hook just never followed it. Both entries
are now ==-pinned, with isort tracking the isort hook's rev.

Verified: `pre-commit run --all-files` is fully green on this branch, and
`pre-commit run flake8 --all-files` reproduces all 10 findings on a clean
origin/main checkout before the pin and passes after it.
@claude

claude Bot commented Aug 30, 2026

Copy link
Copy Markdown

Review

This is a well-scoped, well-documented CI/tooling fix. The root-cause analysis for both issues (unpinned flake8-isort drifting to isort 9.x vs. the rev-pinned isort 6.0.1 hook, and the mypy hook's stub pins drifting from requirements/local.txt) is clear and each claim is backed by a concrete repro/verification step in the PR description. The changes match the description exactly:

  • .pre-commit-config.yaml: flake8 hook now ==-pins flake8-isort==7.0.0 / isort==6.0.1; mypy hook bumps django-stubs 6.0.6→6.1.0 and djangorestframework-stubs 3.17.0→3.18.0.
  • QuerySets.py: 6 new Iterable[Any] declarations to reconcile the try/except LookupError type mismatch that 6.1.0's stricter values_list typing surfaces. These are narrowly scoped, correctly commented, and don't change runtime behavior (all six variables are only ever consumed via __in).
  • The # type: ignore[misc] on the get_model() false positive in test_corpus_canonical_caml_migration.py is narrowly targeted and well-justified — the test is intentionally asserting the model doesn't resolve.
  • Two changelog fragments, correctly following the changelog.d/ convention.

One thing worth flagging (not a blocker)

The mypy hook's own comment (and this PR's stated rationale) is that the ==-pinned stubs MUST match requirements/base.txt + requirements/local.txt. Checking current main:

requirements/local.txt:10:django-stubs==6.0.7
requirements/local.txt:15:djangorestframework-stubs==3.17.1

This PR bumps the hook to django-stubs==6.1.0 / djangorestframework-stubs==3.18.0 — versions that don't exist yet in requirements/local.txt on main (they land with #2265 / #2260). So merging this PR by itself, before those two land, replaces one drift (hook behind requirements: 6.0.6 vs 6.0.7) with another (hook ahead of requirements: 6.1.0 vs 6.0.7) — the invariant the hook comment asserts is still violated in the interim, just in the opposite direction, and the dev/test image would keep type-checking against 6.0.7 while the hook checks against 6.1.0.

The PR description already discloses this ("the end state is only self-consistent once all three land"), and given trunk-based development with same-day merges this is probably an acceptable, deliberate tradeoff to unblock the other three PRs. But it might be worth a one-line note in the mypy hook's comment (or in the merge order) making explicit that there's a brief window where the pin is intentionally ahead of requirements/local.txt, so a future reader doesn't "fix" it by reverting to 6.0.7 to match current main.

Minor

  • Nice catch preserving the flake8-isort ↔ isort version coupling in a comment ("isort here MUST track the isort hook's rev") — this is the kind of thing that will drift again on the next pre-commit autoupdate if not read carefully, but that's inherent to the pre-commit autoupdate model, not something this PR can fully fix.

Testing

No backend/frontend test suite is needed here since the change is CI-tooling + typing-only fixes; the PR's stated verification (pre-commit run --all-files clean on-branch, reproduction of the flake8 failure on a clean origin/main, and reverting each half independently to confirm they're both load-bearing) is appropriate and thorough for this kind of change.

Overall: solid, low-risk fix for a real CI-breaking issue. No security concerns (pure tooling/typing), no functional/runtime behavior changes.

JSv4 pushed a commit that referenced this pull request Aug 30, 2026
The linter job on this PR fails with 10 I001/I005 findings in
opencontractserver/llms/agents/pydantic_ai_agents.py and
opencontractserver/utils/compact_pawls.py — files this PR does not touch. The
failure reproduces identically on a clean origin/main checkout: the flake8
hook's unpinned flake8-isort floated to isort 9.0.1 while the standalone isort
hook stayed rev-pinned to 6.0.1, and the two disagree about repeated
`from X import (...)` statements.

Cherry-picking the pin here so this PR can reach green without waiting on
#2281 to merge. It no-ops once main carries the same change.
JSv4 pushed a commit that referenced this pull request Aug 30, 2026
The linter job on this PR fails with 10 I001/I005 findings in
opencontractserver/llms/agents/pydantic_ai_agents.py and
opencontractserver/utils/compact_pawls.py — files this PR does not touch. The
failure reproduces identically on a clean origin/main checkout: the flake8
hook's unpinned flake8-isort floated to isort 9.0.1 while the standalone isort
hook stayed rev-pinned to 6.0.1, and the two disagree about repeated
`from X import (...)` statements.

Only the flake8 pin is ported. #2281 also bumps this hook file's mypy stub
pins, but that half needs the type fixes that ship with it, so it stays there.

Cherry-picking so this PR can reach green without waiting on #2281 to merge.
It no-ops once main carries the same change.
JSv4 pushed a commit that referenced this pull request Aug 30, 2026
The linter job on this PR fails with 10 I001/I005 findings in
opencontractserver/llms/agents/pydantic_ai_agents.py and
opencontractserver/utils/compact_pawls.py — files this PR does not touch. The
failure reproduces identically on a clean origin/main checkout: the flake8
hook's unpinned flake8-isort floated to isort 9.0.1 while the standalone isort
hook stayed rev-pinned to 6.0.1, and the two disagree about repeated
`from X import (...)` statements.

Cherry-picking the pin here so this PR can reach green without waiting on
#2281 to merge. It no-ops once main carries the same change.
JSv4 pushed a commit that referenced this pull request Aug 30, 2026
The linter job on this PR fails with 10 I001/I005 findings in
opencontractserver/llms/agents/pydantic_ai_agents.py and
opencontractserver/utils/compact_pawls.py — files this PR does not touch. The
failure reproduces identically on a clean origin/main checkout: the flake8
hook's unpinned flake8-isort floated to isort 9.0.1 while the standalone isort
hook stayed rev-pinned to 6.0.1, and the two disagree about repeated
`from X import (...)` statements.

Only the flake8 pin is ported. #2281 also bumps this hook file's mypy stub
pins, but that half needs the type fixes that ship with it, so it stays there.

Cherry-picking so this PR can reach green without waiting on #2281 to merge.
It no-ops once main carries the same change.
JSv4 pushed a commit that referenced this pull request Aug 30, 2026
The linter job on this PR fails with 10 I001/I005 findings in
opencontractserver/llms/agents/pydantic_ai_agents.py and
opencontractserver/utils/compact_pawls.py — files this PR does not touch. The
failure reproduces identically on a clean origin/main checkout: the flake8
hook's unpinned flake8-isort floated to isort 9.0.1 while the standalone isort
hook stayed rev-pinned to 6.0.1, and the two disagree about repeated
`from X import (...)` statements.

Cherry-picking the pin here so this PR can reach green without waiting on
#2281 to merge. It no-ops once main carries the same change.
JSv4 pushed a commit that referenced this pull request Aug 30, 2026
The linter job on this PR fails with 10 I001/I005 findings in
opencontractserver/llms/agents/pydantic_ai_agents.py and
opencontractserver/utils/compact_pawls.py — files this PR does not touch. The
failure reproduces identically on a clean origin/main checkout: the flake8
hook's unpinned flake8-isort floated to isort 9.0.1 while the standalone isort
hook stayed rev-pinned to 6.0.1, and the two disagree about repeated
`from X import (...)` statements.

Only the flake8 pin is ported. #2281 also bumps this hook file's mypy stub
pins, but that half needs the type fixes that ship with it, so it stays there.

Cherry-picking so this PR can reach green without waiting on #2281 to merge.
It no-ops once main carries the same change.
JSv4 pushed a commit that referenced this pull request Aug 30, 2026
The linter job on this PR fails with 10 I001/I005 findings in
opencontractserver/llms/agents/pydantic_ai_agents.py and
opencontractserver/utils/compact_pawls.py — files this PR does not touch. The
failure reproduces identically on a clean origin/main checkout: the flake8
hook's unpinned flake8-isort floated to isort 9.0.1 while the standalone isort
hook stayed rev-pinned to 6.0.1, and the two disagree about repeated
`from X import (...)` statements.

Cherry-picking the pin here so this PR can reach green without waiting on
#2281 to merge. It no-ops once main carries the same change.
JSv4 pushed a commit that referenced this pull request Aug 30, 2026
The linter job on this PR fails with 10 I001/I005 findings in
opencontractserver/llms/agents/pydantic_ai_agents.py and
opencontractserver/utils/compact_pawls.py — files this PR does not touch. The
failure reproduces identically on a clean origin/main checkout: the flake8
hook's unpinned flake8-isort floated to isort 9.0.1 while the standalone isort
hook stayed rev-pinned to 6.0.1, and the two disagree about repeated
`from X import (...)` statements.

Only the flake8 pin is ported. #2281 also bumps this hook file's mypy stub
pins, but that half needs the type fixes that ship with it, so it stays there.

Cherry-picking so this PR can reach green without waiting on #2281 to merge.
It no-ops once main carries the same change.
CI (Python 3.12) reported the `# type: ignore[misc]` on
test_no_corpus_description_revision_model as an unused ignore, while a
cold-cache run on 3.11 needs it — mypy.ini sets warn_unused_ignores, so the
comment cannot be right in both. The plugin only resolves *literal* get_model()
string pairs, so the model name now lives in a str-annotated local instead.
Neither interpreter's plugin has a literal to match, and the test's runtime
behavior and assertion are untouched.

Verified with a cleared .mypy_cache: `pre-commit run --all-files` is fully green.
@claude

claude Bot commented Aug 30, 2026

Copy link
Copy Markdown

Reviewed the diff (.pre-commit-config.yaml, QuerySets.py, the migration test, and the two changelog fragments).

Overall: this is a tight, well-scoped tooling fix. The root-cause analysis in the description checks out against the diff, and the fix pattern for the flake8-isort pin correctly mirrors the existing mypy-hook convention (unpinned additional_dependencies float on every hook-env rebuild). A few notes, nothing blocking:

Correctness

  • flake8-isort==7.0.0 + isort==6.0.1 pin matches the isort hook's rev: 6.0.1 at .pre-commit-config.yaml:51 — the "must track the isort hook's rev" invariant the comment asserts is actually true in this diff, not just asserted.
  • The QuerySets.py changes are pure type annotations (permitted_ids: Iterable[Any] etc.) added ahead of try/except blocks whose two arms already produced runtime-equivalent values (a lazy values_list queryset vs. []). No behavioral change — confirmed by reading the surrounding visible_to_user methods; the annotated variables are only ever consumed via __in/pk__in lookups, so Iterable[Any] is an accurate, honest common type rather than a type-checker-appeasing lie.
  • The test_corpus_canonical_caml_migration.py fix (hoisting the literal into a str-annotated local) is a reasonable way to dodge django-stubs' static get_model() literal-pair resolution without relying on # type: ignore, whose necessity is correctly noted as interpreter-version-dependent (would trip warn_unused_ignores on 3.12 while being required on 3.11). Good catch avoiding a fragile ignore-comment fix.

Minor

  • Small inconsistency between the PR description (says the test fix is at "test_corpus_canonical_caml_migration.py:161") and the changelog fragment (changelog.d/mypy-hook-stub-parity.changed.md, says line 157). The diff hunk confirms 157 is correct (start of test_no_corpus_description_revision_model); purely cosmetic, not worth a respin on its own.
  • The six Iterable[Any] annotations in QuerySets.py are a bit repetitive (same 4-line comment block pattern x4). Given they annotate genuinely different try/except pairs across two separate methods (DocumentQuerySet.visible_to_user and AnnotationQuerySet.visible_to_user), duplicating the explanation locally is more readable than forcing a shared helper — reasonable tradeoff, no change requested.

Test coverage: No new tests added, but none are needed — this is a CI/type-checking config fix plus type-annotation-only production code changes and a test-only fix for a stub-resolution false positive. The existing test suite plus pre-commit run --all-files (documented in the test plan) is the right verification surface here.

Security: N/A — no security-relevant surface touched.

Nice work isolating a flaky-looking CI failure ("red on main with no commit responsible") down to the actual mechanism (unpinned transitive additional_dependencies drift) rather than just re-running CI or loosening the lint rule.

JSv4 pushed a commit that referenced this pull request Aug 30, 2026
backend.yml's linter job runs mypy TWICE: the pinned pre-commit hook, and then
a second authoritative `python -m mypy --config-file mypy.ini` (backend.yml:107)
against whatever requirements/local.txt installs. So bumping django-stubs there
does gate CI, and 6.1.0 surfaces 7 errors this PR would otherwise land red with:

  QuerySets.py x6 - six guardian-permission id lists are a lazy `values_list`
  queryset in a `try` and `[]` in the matching `except LookupError`. 6.1.0 types
  `values_list(..., flat=True)` as `QuerySet[Model, int]`, which no longer
  unifies with `list[Never]`. Each now carries an explicit `Iterable[Any]`
  declaration — they are only ever consumed by an `__in` lookup.

  test_corpus_canonical_caml_migration.py x1 - the test asserts the model is
  GONE, so the lazy reference is unresolvable on purpose; 6.1.0's plugin
  resolves literal get_model() string pairs statically and errors on a miss.
  The name moves into a str-annotated local so there is no literal to match.
  Whether the error fires varies by interpreter and mypy version, which is why
  a `# type: ignore` is not used here.

Typing-only; no runtime behavior changes. No-ops once main carries #2281.
JSv4 pushed a commit that referenced this pull request Aug 30, 2026
backend.yml's linter job runs mypy TWICE: the pinned pre-commit hook, and then
a second authoritative `python -m mypy --config-file mypy.ini` (backend.yml:107)
against whatever requirements/local.txt installs. So bumping django-stubs there
does gate CI, and 6.1.0 surfaces 7 errors this PR would otherwise land red with:

  QuerySets.py x6 - six guardian-permission id lists are a lazy `values_list`
  queryset in a `try` and `[]` in the matching `except LookupError`. 6.1.0 types
  `values_list(..., flat=True)` as `QuerySet[Model, int]`, which no longer
  unifies with `list[Never]`. Each now carries an explicit `Iterable[Any]`
  declaration — they are only ever consumed by an `__in` lookup.

  test_corpus_canonical_caml_migration.py x1 - the test asserts the model is
  GONE, so the lazy reference is unresolvable on purpose; 6.1.0's plugin
  resolves literal get_model() string pairs statically and errors on a miss.
  The name moves into a str-annotated local so there is no literal to match.
  Whether the error fires varies by interpreter and mypy version, which is why
  a `# type: ignore` is not used here.

Typing-only; no runtime behavior changes. No-ops once main carries #2281.
@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@JSv4
JSv4 merged commit fbe422d into main Aug 30, 2026
15 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 30, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants