Skip to content

fix: give the mypy gate the same environment locally and in CI - #668

Merged
johanzander merged 2 commits into
mainfrom
fix/mypy-gate-environment-parity
Aug 21, 2026
Merged

fix: give the mypy gate the same environment locally and in CI#668
johanzander merged 2 commits into
mainfrom
fix/mypy-gate-environment-parity

Conversation

@johanzander

@johanzander johanzander commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Found while cutting the v10.1.0b12 beta release: the release PR went red on
Code quality → mypy on changed files with 26 "new" type errors, none of them
from new code.

Two separate causes

1. The gate's CI environment does not match its local one.

The Code quality job installed black ruff mypy and nothing else, while
quality-check.sh runs through a .venv that has the full dependency set.
With --ignore-missing-imports, anything absent silently becomes Any. The
step's own comment promises the opposite — "Same script quality-check.sh
runs, so a green local gate and a green CI gate mean the same thing."

The divergence runs in both directions, which is why annotating cannot
close it. All figures reproduced locally with mypy --no-site-packages
against the gate's own flags:

file missing package without with effect
backend/tests/test_agent_permissions.py pytest 7 0 annotating converts no-untyped-defuntyped-decorator (7 → 5, never 0)
backend/api.py fastapi 53 43 10-error delta, entirely untyped-decorator on @router.get
core/bess/pwl_window_dp.py numpy 3 7 masks 4 real errors — a genuine mistake clears the merge gate

The numpy row is the dangerous one: the gate was not just noisy, it was
letting real type errors through in optimizer code.

Fixed by installing backend/requirements.txt and requirements-dev.txt
— the same two files every other Python job in this workflow installs
(lines 111-114, 151-154).

2. Files that predate the gate were never charged.

The ratchet only ever compares a file against its own merge-base, so files
merged before #614 landed carried their untyped functions for free — until a
release PR compared them against a stale mirror and every one of them read as
new. Annotated the ones with no baseline:

  • backend/tests/test_agent_permissions.py and
    core/bess/tests/unit/test_vpp_idle_at_reserve_floor.py are new files, so
    every error in them counts. Both are now clean.
  • core/bess/simulation/vpp_simulator.py's _simulate gains the callback
    type its docstring already describes:
    Callable[[int, float], VppCommand].
  • The four functions fix: release VPP control when IDLE at the reserve floor (#592) #619 added to test_vpp_simulator_branches.py and the
    two it added to test_solax_modbus_growatt_vpp.py get return types.

The pre-existing untyped functions in those last two files are deliberately
left alone (40 and 16 respectively) — the ratchet does not charge them, and
burning them down is separate work.

One real defect surfaced

Narrowing bsm._inverter_controller past its | None exposed a mismatch the
union-attr error had been masking: current_schedule is typed DPSchedule,
and the test assigns a SimpleNamespace stub. Cast, with a note that only
.actions is read on that path.

Known and deliberately not fixed here

  • pyproject.toml's module = ["tests.*"] override never matches
    anything.
    mypy says so on every run (note: unused section(s)), because
    real test modules resolve as core.bess.tests.* and backend.tests.*. So
    test files are subject to disallow_untyped_defs despite the project
    explicitly deciding they should not be — which is what generated the
    annotation churn in this PR. Correcting the pattern would retire the whole
    class, but it changes what the gate enforces across every test file in the
    repo, so it is the maintainer's call rather than a release-unblocking fix.

    Do not start annotating those two files by hand before that decision is
    made.
    All 56 remaining errors are no-untyped-def in test files — every
    one of them exists solely because that override never matches. Correcting
    the pattern makes the "40 and 16" burn-down disappear on its own, so the
    effort would be wasted.

  • Nothing in requirements-dev.txt is version-pinned, so the gate stays
    exposed to upstream black/ruff/mypy releases. Pre-existing; not touched.

Verification

  • pytest -m "not slow" — 2193 passed, 50 skipped
  • black --check . / ruff check . — clean
  • mypy on the five files: 56 errors remain, all pre-existing and matching the
    baseline exactly (42→40, 20→16), so the ratchet sees zero new

No CHANGELOG entry: type annotations and a CI install line, no user-visible
effect.

🤖 Generated with Claude Code

https://claude.ai/code/session_017gRPJGGJvK1ZH5DjpZnxSJ

johanzander and others added 2 commits August 21, 2026 19:36
The Code quality job installed `black ruff mypy` and nothing else, so mypy
resolved `pytest` to Any there while the local `.venv` had the real package.
That divergence is the opposite of what the step promises ("a green local
gate and a green CI gate mean the same thing"), and it is not closeable by
annotating: with an untyped `pytest`, annotating a decorated test function
only converts `no-untyped-def` into `untyped-decorator`. Measured on
test_agent_permissions.py -- 6 errors before, 5 after. Install the dev
requirements instead, which is where pytest, black and ruff are already
pinned.

With the environments matched, annotate the functions the ratchet had no
baseline for. These files predate the gate (#614), so nothing charged them
until a release PR compared them against a stale mirror:

- test_agent_permissions.py and test_vpp_idle_at_reserve_floor.py are new
  files, so every error in them counts; both are now clean.
- vpp_simulator.py's `_simulate` gains the callback type its docstring
  already describes.
- the four functions #619 added to test_vpp_simulator_branches.py and the
  two it added to test_solax_modbus_growatt_vpp.py get return types. The
  pre-existing untyped functions in those two files are left alone -- the
  ratchet does not charge them, and burning them down is separate work.

Narrowing `_inverter_controller` surfaced a real mismatch the `| None` error
had been masking: `current_schedule` is a `DPSchedule`, and the test assigns
a `SimpleNamespace` stub. Cast it, with a note that only `.actions` is read.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gRPJGGJvK1ZH5DjpZnxSJ
Installing only requirements-dev.txt closed the divergence for `pytest` and
left the identical one for everything in backend/requirements.txt. Every
other Python job in this workflow installs both files; this one now does too.

It cuts both ways, so neither half is optional:

- Missing `fastapi` makes `@router.get` untyped exactly as missing `pytest`
  made `@pytest.fixture` untyped. backend/api.py reports 53 errors without
  site-packages against 43 with, and the 10-error delta is entirely
  `untyped-decorator` -- so a new annotated endpoint would pass locally and
  fail here, unfixable by annotating.
- Missing `numpy` MASKS errors instead. core/bess/pwl_window_dp.py reports
  7 errors with it installed and 3 without, so a genuine type error in a
  numpy-using optimizer file would clear the merge gate and surface only on
  the maintainer's machine.

Found in review of the first commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gRPJGGJvK1ZH5DjpZnxSJ
@bess-agent

Copy link
Copy Markdown
Collaborator

@claude-bot review

@bess-manager-claude-bot bess-manager-claude-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Fixes the CI/local environment divergence in the mypy gate: .github/workflows/ci.yml's quality job installed only black ruff mypy, while scripts/quality-check.sh (and every other Python job in the same workflow) installs backend/requirements.txt + requirements-dev.txt. With --ignore-missing-imports, missing packages silently become Any, which cut both ways: missing pytest/fastapi produced false positives (untyped-decorator), and missing numpy masked real errors in core/bess/pwl_window_dp.py (3 errors reported vs 7 with numpy installed — a genuine defect that would have cleared the merge gate).

Root cause matches evidence. I reproduced the PR's own numbers locally on a fresh venv from the PR branch:

  • black --check . — clean
  • ruff check . — clean
  • scripts/mypy-changed.sh .venv/bin/mypy --include-worktree✅ mypy OK (no new errors; 56 pre-existing in touched files), matching the description's 56-remaining/zero-new claim exactly
  • pytest -m "not slow"2193 passed, 50 skipped, matching the description exactly
  • gh pr checks 668 — all 8 required checks green, including Merge gate

Fix is minimal and matches the stated pattern. .github/workflows/ci.yml:461-463 now installs the same two files as the test-fast (ci.yml:111-114) and test-algorithm jobs — no new abstraction, just environment parity with what already exists elsewhere in the same file. The added comment (ci.yml:454-465) documents the WHY (the numpy-masking asymmetry) rather than restating what the code does, consistent with the comment rule.

Second cause (pre-#614 files never charged by the ratchet) is handled correctly and conservatively. New files (backend/tests/test_agent_permissions.py, core/bess/tests/unit/test_vpp_idle_at_reserve_floor.py) are fully annotated since they have no baseline and the ratchet counts every error in them. Pre-existing untyped functions in test_vpp_simulator_branches.py / test_solax_modbus_growatt_vpp.py are left alone with an explicit note that burning down the 40/16 backlog is separate work — correctly scoped, no drive-by cleanup.

One real bug surfaced and fixed correctly, not routed around. core/bess/tests/unit/test_vpp_idle_at_reserve_floor.py:63-68: narrowing bsm._inverter_controller past | None exposed that current_schedule is typed DPSchedule but the test assigns a SimpleNamespace. Handled with cast(DPSchedule, SimpleNamespace(actions=[0.0] * 96)) plus a WHY comment ("only .actions is read on this path") — this is a type-level fix to a test stub, not a route-around of a real dependency-ordering problem, so the workaround check doesn't apply here.

Test coverage: N/A in the traditional sense — this PR is CI config + type annotations, not new runtime behavior, so there's no new regression-catching test to evaluate. The CI change is self-verifying: the "Code quality" job passing on this PR (Code quality pass 54s) is itself the proof the divergence is closed, since the same job would have failed under the old pip install black ruff mypy line given the numpy/pytest/fastapi deltas documented in the PR body.

Scope: Diff touches exactly 6 files, all directly implicated (workflow install step + type annotations on the specific functions/files the ratchet now charges). No Optional[x], hasattr/getattr fallbacks, or new classes introduced. No CHANGELOG entry, which is appropriate — CI tooling and type annotations only, no user-visible effect.

No blockers. Approving.

@johanzander
johanzander marked this pull request as ready for review August 21, 2026 19:12
@johanzander
johanzander merged commit b2a0107 into main Aug 21, 2026
8 checks passed
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.

2 participants