Skip to content

ci(pyright): gate on the rules a file decides for itself - #2092

Merged
lmeyerov merged 3 commits into
masterfrom
feat/ci-pyright-ratchet
Sep 17, 2026
Merged

lmeyerov merged 3 commits into
masterfrom
feat/ci-pyright-ratchet

Conversation

@lmeyerov

@lmeyerov lmeyerov commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Takes over #1123 (@JithinBathula) and finishes it into an enforceable gate. Closes #1075.

#1123 added bin/pyright.sh and a pyrightconfig.json, and stopped there by design — the author listed CI integration and rule enforcement as non-goals. Two things had to be settled before it could gate: the tool was unpinned, and the config change was a net loosening with exactly one tightening. Both are resolved here. The contributor's bin/pyright.sh resolution order and pyrightconfig.json are kept.

The problem: most pyright rules are not reproducible

Pyright was run over one unchanged tree in five environments — python 3.8 (pandas 2.0.3), 3.11, 3.12 (the CI lint lockfile), 3.14 (pandas 3.0.5), and a workstation carrying polars 1.42, cudf 25.10, scipy and scikit-learn.

rule py3.8 py3.12 CI py3.14 py3.11 +polars workstation
reportAttributeAccessIssue 776 778 778 146 810
reportOptionalMemberAccess 187 183 183 180 195
reportCallIssue 146 143 143 146 8
reportReturnType 10 7 7 23 2
reportInvalidTypeForm 35 35 35 34 1201
reportUndefinedVariable 195 195 195 195 195
reportPossiblyUnboundVariable 61 61 61 61 61
reportUnusedExpression 15 15 15 15 15

Gating on a rule from the top half means CI and the developer who ran the same command disagree, and the baseline drifts on every pandas release.

What gates

Five rules, admitted on a principle rather than a measurement: the rule must be decided by the source file's own control flow, names and syntax, with no type consulted. reportPossiblyUnboundVariable, reportUndefinedVariable, reportUnsupportedDunderAll, reportUnusedExpression, reportSelfClsParameterName.

The principle is doing real work here, because measurement alone was not enough: reportOptionalSubscript, reportTypedDictNotRequiredAccess and reportIndexIssue produced identical per-file counts across the first four environments and then diverged on the fifth. They are rare enough to agree by luck. Everything else is still collected and shown by --report; it just never gates.

Shape

Deliberately the same as bin/ci_type_hygiene_guard.py and bin/ci_comment_density_guard.py, down to the flags:

  • bin/pyright.sh pins pyright 1.1.414, in both the uvx and npx fallbacks, and uses a local install only when it is that exact version — otherwise it says so and fetches the pinned build. An unpinned tool moves the baseline underneath the repo.
  • bin/ci_pyright_guard.py ratchets per-file counts against bin/ci_pyright_baseline.json: counts may shrink, never grow; a file absent from the baseline must be clean. --report / --list / --strict / --update-baseline / --from-json.
  • python-pyright runs it on py3.12 only. Putting it in bin/lint.sh would pay 21s plus a tool fetch in all seven matrix cells for output the table above shows is byte-identical in all seven.

Two things beyond the rule counts also gate, both added after reviewing the first commit:

  • A collapsed gate fails. Scope lives in pyrightconfig.json, a different file from the baseline, so widening an exclude silenced the gate with no baseline change and no failure — appending graphistry/compute dropped the gated findings from 273 to 56 and still exited 0, while printing "10 file(s) now below baseline". The baseline now records the file count it was built over, and a run that sees materially less fails. A real scope change still goes through --update-baseline, where the delta shows up in review.
  • A file that does not parse fails. Pyright reports those with no rule at all, so they were routed to the ungated pile and could never fail — and an unparseable file yields fewer gated findings, so it read as an improvement. Parse failures consult no type either, so they are gated under <unparseable> and must stay at zero.

273 findings are grandfathered, of which 195 are one module — graphistry/compute/gfql/cypher/projection_planning.py builds its namespace with globals().update(vars(_lowering)) and already carries # mypy: ignore-errors and # ruff: noqa: F821. Its count is left visible rather than excluded, so that fixing the module shows up as slack under --strict. That module being invisible to all three checkers is worth its own issue; I did not fold it in here.

Verification

  • End to end: adding if flag: edge_map = 1 / return edge_map to graphistry/util.py turns the gate red with "edge_map" is possibly unbound — the exact case from feat: add pyright script for type checking with configuration #1123's motivation — and green again on revert.
  • One baseline, five environments, exit 0 in all five.
  • 19 tests pin the ratchet. A 13-mutant battery: 12 caught, and the 2 that survived were verified inert on their own (the comparison loop never reads the keys they add) and caught when combined.
  • Run locally before pushing: bin/lint.sh (ruff + both guards), bin/typecheck.sh (mypy clean, 349 files), actionlint, zizmor (no new findings), and changed_line_coverage.py --min-percent 80 (0 eligible changed lines — this PR touches bin/, workflows, docs and a test).

Note for the reviewer

python-pyright is a new job, so it is not in branch protection's required checks — worth adding there if this lands.

Co-authored with @JithinBathula's work from #1123.

🤖 Generated with Claude Code

https://claude.ai/code/session_017ropeBMLJUuy6ViYwy15ud

lmeyerov and others added 3 commits September 16, 2026 22:12
Pyright catches what ruff and mypy miss: locals bound on only some paths,
names that resolve nowhere, statements with no effect. Running it is easy;
gating on it is not, because most of its rules read third-party stubs.

Measured over five environments (python 3.8/3.11/3.12/3.14, and a workstation
carrying polars, cudf, scipy and scikit-learn) on one unchanged tree,
reportAttributeAccessIssue ranges from 146 to 810 findings, reportReturnType
from 2 to 78. A gate on those fails for whoever has cudf installed and passes
in CI. So only five rules gate -- the ones decided by a file's own control
flow, names and syntax -- and the rest are reported but never enforced.

Agreement across environments was necessary but not sufficient evidence:
reportOptionalSubscript, reportTypedDictNotRequiredAccess and reportIndexIssue
all matched across four environments and then diverged on the fifth, being rare
enough to agree by luck. The admission bar is the principle, not the sample.

- bin/pyright.sh pins pyright 1.1.414, and uses a local install only when it
  is that version; an unpinned tool moves the baseline underneath us.
- bin/ci_pyright_guard.py ratchets per-file counts against
  bin/ci_pyright_baseline.json, the same shape as the type-hygiene and
  comment-density guards: counts may shrink, never grow, and a file absent
  from the baseline must be clean.
- python-pyright runs it on py3.12 only. All seven lint matrix cells would
  produce byte-identical output for these rules.

273 findings are grandfathered. 195 of them are one module that builds its
namespace with globals().update(vars(...)) and already carries
`# mypy: ignore-errors` and `# ruff: noqa: F821`; its count is left visible
rather than excluded so that fixing it shows up as slack under --strict.

Builds on #1123 by Jithin Bathula, whose bin/pyright.sh and pyrightconfig.json
this keeps. Closes #1075.

Co-Authored-By: Jithin Bathula <jithin.bathula@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ropeBMLJUuy6ViYwy15ud
Review of the previous commit found the ratchet could go quiet without anyone
noticing, which defeats its purpose.

Scope lives in pyrightconfig.json, a different file from the baseline, so
widening an exclude silenced the gate with no baseline change and no failure:
appending "graphistry/compute" to exclude dropped the gated findings from 273
to 56 and still exited 0 -- while printing "10 file(s) now below baseline". The
guard observed the collapse and passed anyway. Shrinking findings and shrinking
scope are indistinguishable from counts alone, so the baseline now records the
file count it was built over and a run that sees materially less fails. A real
scope change is still fine; it just has to go through --update-baseline, where
the delta shows up in review.

Pyright reports a file it cannot parse with no rule at all, so those findings
were routed to the ungated pile and could never fail. An unparseable file also
yields fewer gated findings, so it read as an improvement. Parse failures
consult no type either, so they are gated under <unparseable> and must stay at
zero; the tree has none today.

Also aligns Finding with the sibling guard's `@dataclass(frozen=True)` and
real annotations rather than a hand-rolled __slots__ class and `# type:`
comments.

Verified: exclude-widening and a fabricated zero-file report now exit 1; an
unparseable file exits 1 naming it; the injected possibly-unbound local still
exits 1 and reverts to 0; one baseline still passes in all five environments.
19 tests, 13-mutant battery -- 12 caught, the 2 survivors inert alone and
caught in combination.

Co-Authored-By: Jithin Bathula <jithin.bathula@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ropeBMLJUuy6ViYwy15ud
…blings

63 lines re-derived the environment measurement in prose; that argument lives
in the PR and does not need repeating for someone reaching for the commands.
42 lines now, next to Type Hygiene at 39 and Comment Density at 43.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ropeBMLJUuy6ViYwy15ud
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.

tooling: explore adding pyright in monitor mode to catch possibly-unbound variables

1 participant