M-TYPECHECK: restore real type checking in CI - #53
Merged
Conversation
The mypy pre-commit hook is pinned `stages: [pre-push]` (kept off every commit — it's slow), and CI's "Lint & type check" job ran `pre-commit run --all-files`, which only executes default-stage hooks. mypy therefore never ran — not locally on commit, not in CI — despite the job name, step name, and branch-protection check all claiming type checking was happening. A missing job is visible; a green one that silently skipped its work is not. TYPE.1 (measured): 78 real errors via the pinned hook's isolated venv (mypy v1.10.0 + numpy + types-requests only — this is what a fresh CI runner actually sees; my shell's pip-installed mypy 2.3.1 disagreed, both on count and on python_version=3.9 support, which is exactly the "open floor" drift the project already guards against for black/ruff). TYPE.2: mypy gets its own explicit CI step (pinned mypy~=1.10.0, matching deps/scope) instead of routing through stage-filtered pre-commit — a dedicated step has no stage-filtering machinery for a future hook to silently hide behind. Also pinned mypy in the dev extra (was an open floor) for the same reason black/ruff are pinned there. TYPE.3 (78 -> 0, no baseline needed): - app.py: 45 attr-defined errors, all one mechanical pattern — the TYPE_CHECKING widget-attribute stub block had drifted out of sync with companion builder modules over many sessions (not just MET.5's 3 new widgets). Backfilled all 32 missing declarations near their feature groups; verified each against a real runtime attribute (one, the copy-data buttons, is built via setattr() in a loop, hence invisible to a static grep — confirmed live on a real QuantUIApp instance). - benchmarks.py: `res` genuinely holds one of 3 unrelated result types across branches (annotated Any, each branch already reads only its own type); _TeeStream/progress_stream and the live_message/step kwargs are deliberate duck-typing/feature-detection patterns needing a narrow cast/type-ignore, not a redesign. - Untyped-dict return sites (_CALC_TYPE_KEYS, _XC_ALIAS) got real Dict[str, str] annotations rather than casts — an actual type-safety improvement. Third-party-without-stubs returns (plotly, py3Dmol, pyscf) got documented casts. results_storage.py's existing `type: ignore[...]` comments were missing error codes mypy 1.10 actually reports (a scoped ignore only suppresses the codes listed). - orbital_visualization.py: bond_x/y/z's None entries are a deliberate Plotly line-break convention — the declared type was just too narrow (List[float] -> List[Optional[float]]). - infer_charge_and_spin(): its own docstring documents returning (0, 0) for a None mol_atom/mo_occ "so callers can pass through directly without a separate None-check" — the type hint just didn't say Optional. First attempt at this (widening the caller's guard to bail on None) was WRONG: it changed real behavior and broke 2 tests that exercise exactly that documented pass-through path. Reverted; fixed the hint instead — the correct, non-behavioral fix for a caller/callee contract mismatch mypy surfaced. - app_builders.py: `app.attr: type = value` (annotating a non-self attribute assignment) isn't valid syntax for mypy — my own MET.5 spin- helper commit had this; removed the annotation. TYPE.4: not needed — reached a clean bill, so "Lint & type check" already names what it does. TYPE.5: new tests/test_ci_hook_reachability.py — walks every .pre-commit-config.yaml hook and asserts any hook restricted off the default stage is independently invoked in ci.yml's actual run: command bodies (not a step name/comment mentioning it, not the generic `pre-commit run --all-files` line). Verified both directions: fails against a reconstruction of the original bug, and separately fails against a step merely *named* "Type check (mypy)" with an unrelated run: body — a gap the first draft of this test had. Full no-network suite: 2555 tests, 14 failed (pre-existing, env-only — pyscf-properties uninstallable in the cloud container), 0 errors. Same count before and after. ruff+black+mypy all clean via the actual pinned pre-commit hooks. Contributions: - Claude (Opus 4.8): CI wiring, all 78 fixes, regression test, self-caught regression - Jonathan Schultz: overall vision, planning, review, and orchestration Co-authored-by: Jonathan Schultz <nccu-schultz-lab@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
NCCU-Schultz-Lab
marked this pull request as ready for review
August 20, 2026 20:49
This was referenced Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second in the stack behind M-METAL (#52, now merged), so this is rebased on
mainand green. Next up: #EXPORT2 → #CHECKPOINT.pre-commit run --all-fileswas silently skipping mypy the whole time: the hook was pinnedstages: [pre-push], which a default-stage-only runner never reaches. mypy now has its own explicit CI step, independent of the pre-commit hook stage.tests/test_ci_hook_reachability.py) that guards this exact class of bug from recurring (a hook pinned to a stage the runner never invokes).mypy~=1.10.0pinned in the dev extras so the authoritative check (pre-commit run --hook-stage pre-push mypy --all-files, an isolated venv with mypy 1.10.0 + numpy + types-requests) matches what a fresh CI runner actually sees.None-guard change would have brokeninfer_charge_and_spin's documented(0, 0)-for-Nonecontract. Caught by the full test suite before push, reverted, and root-caused instead (the function's type hint was just missingOptional) rather than papered over.Test plan
pytest -m "not network"— same 14 pre-existing NMR failures, 0 errors.pre-commit run --hook-stage pre-push mypy --all-files— clean (re-confirmed locally on themain-rebased branch).mainbase: Lint & type check + tests on ubuntu 3.9/3.10/3.11 + windows 3.11.