From e62d217c6ed5c392147af9354229f2551b9ff8bf Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 23:41:10 -0400 Subject: [PATCH] ci: wire the existing linters into CI as advisory checks The Makefile has carried `vermin`, `pylint`, `mypy` and `bandit` targets all along and no workflow ran any of them, so nothing noticed when their output changed. (`isort` does appear in cron-vendor.yml, but as a formatter that rewrites the generated constants, not as a check.) Requested by the owner off the back of the CONTRIBUTING refresh in 4ecac90a1, which established that the local linters are not a CI gate; no issue tracks it. - .github/workflows/lint.yml: new workflow, one job on Python 3.14, running all four tools as advisory steps. Measured on 15189abff before choosing that: bandit 8 findings (7 medium, 1 low, 0 high); mypy 115 errors in 39 files of 496 checked; vermin 105 files flagged; pylint 5902 messages (80 E, 4767 W, 519 R, 541 C). None of the four is clean, so none can block today -- a job that is red the day it lands teaches everyone to scroll past red, which costs more than the checks are worth. Each step writes its count to the run summary instead, and promoting one to blocking is deleting its `continue-on-error` line. The header records the promotion order and why pylint is not in it: 4358 of its 4767 warnings are `unused-argument`, `redefined-builtin` and `super-init-not-called`, all inherent to the schema DSL. - One job rather than four, and one interpreter rather than a matrix: the tools read the source, so five runs would yield five copies of the same findings, and four jobs would pay for four checkouts and four `.[all]` installs to run four cheap tools. 3.14 matches cron-vendor.yml and is the version the recorded counts came from, so a count that moves means the code moved. - Triggers are `pull_request`, a Saturday 06:00 schedule and `workflow_dispatch` -- deliberately not `push: [main]`, which the other validation workflows here use. Those gate something; these gate nothing yet, so a second advisory run per merge, on a commit that already got one on its pull request, would add no information while competing for runners that pull requests here already queue behind 21 checks for ten minutes at a time. The weekly schedule covers main's own drift, which is all the push trigger would have added. - Makefile: the four flag sets become variables and `pipenv run` becomes `$(RUN)`, so the workflow runs `make pylint RUN=` rather than restating the flags. That leaves exactly one definition of each tool's flag set, so local and CI cannot drift into "clean on my machine, red in CI". Verified the expanded commands are byte-identical to the literals they replace. New `vermin-ci` target because `vermin` redirects into temp/ and hands the file to an editor, so its exit status is the editor's and a run that found violations still succeeds; `vermin-ci` writes to stdout and lets the failure propagate, off the same `$(VERMIN_FLAGS)`. No linter configuration was weakened to make anything pass: every tool runs the flags the Makefile already used, and `vermin.ini` is picked up from the repository root locally and in CI alike. Two findings are reported rather than fixed, since a lint fix does not belong in a CI wiring change. vermin puts the code's real floor at 3.11 (`enum.StrEnum` and `enum.show_flag_values`, with 86 modules importing `typing_extensions`) while `vermin.ini` targets 3.6 and `pyproject.toml` still declares `requires-python = ">=3.6, <4"` -- the declared floor and the actual one are five releases apart, and that is why vermin cannot block. And mypy finds `Optional`, `Protocol` and `Any` used in string annotations that are never imported, at pcapkit/protocols/schema/internet/ipv6_route.py:271 and :136 and pcapkit/utilities/logging.py:350; latent, because `cast()`'s first argument is a string Python never evaluates. No changelog entry: CI and developer tooling, not user-visible. Verified `python util/changelog_md.py --check` still exits 0. No tests run -- nothing here touches the package. --- .github/workflows/lint.yml | 198 +++++++++++++++++++++++++++++++++++++ Makefile | 35 ++++++- 2 files changed, 229 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/lint.yml diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 0000000000..a7db699d83 --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,198 @@ +name: "Lint" + +# The four linters this repository already maintains -- vermin, pylint, mypy and +# bandit -- had Makefile targets and no CI presence at all, so nothing noticed +# when their output changed. (`isort` does appear in cron-vendor.yml, but as a +# formatter that rewrites the generated constants, not as a check.) This workflow +# is where they run. +# +# Every one of them is advisory -- `continue-on-error: true` on each step -- and +# that is a measurement result rather than a preference. Counts on 15189abff, +# each from the same flag sets the Makefile uses: +# +# bandit 8 findings (7 medium, 1 low, 0 high) exit 1 +# mypy 115 errors in 39 files (496 checked) exit 1 +# vermin 105 files flagged; needs 3.11, targets 3.6 in vermin.ini exit 1 +# pylint 5902 messages (80 E, 4767 W, 519 R, 541 C) exit 30 +# +# None of the four is clean, so none can block today: a job that is red the day +# it lands teaches everyone to scroll past red, which costs more than the checks +# are worth. Advisory steps keep the numbers visible (each writes its count to +# the run summary) without that damage, and promoting one to blocking is a matter +# of deleting its `continue-on-error` line once its count reaches zero. +# +# Promotion order, cheapest first: +# * bandit -- 8 findings, and 7 are B104 "binding to all interfaces" on +# `address: ... = '0.0.0.0'` *default arguments* in MH/MIP option builders, +# which bind no socket. This package already uses `# nosec` (16 findings are +# suppressed that way today), so triaging 8 comments is the whole job. +# * mypy -- 24 of the 115 are `unused-ignore`, i.e. `# type: ignore` comments +# that are no longer needed. Deleting those is risk-free and cuts a fifth. +# * vermin -- blocked on a decision, not on cleanup: see the step below. +# * pylint -- not realistically promotable at this rule set. 4358 of its 4767 +# warnings are `unused-argument` (2931), `redefined-builtin` (808) and +# `super-init-not-called` (619), all three of which are inherent to the +# schema DSL: protocol fields are legitimately named `next`/`type`/`id`, and +# the `if TYPE_CHECKING: def __init__(...)` stubs exist precisely so as not +# to call `super().__init__`. Narrowing to `--enable=E,F` does not rescue it +# either -- that is still 80 errors, mostly `unsubscriptable-object` (40) and +# `no-member` (22) false positives off the same metaprogramming. + +on: + # Deliberately *not* `push: [main]`, which is the trigger the other validation + # workflows here use. Those gate something: a failing unit-test or CodeQL run + # on main is a commit someone must look at. These steps gate nothing yet, so a + # second advisory run per merge -- on a commit that already got one on its pull + # request -- would buy no information while competing for runners that are + # visibly oversubscribed (pull requests on this repository have been sitting at + # 21 queued checks for ten minutes at a time). The weekly schedule covers + # main's own drift, which is the only thing the push trigger would have added. + pull_request: + branches: [main] + schedule: + # Saturdays, an hour clear of the other weekly jobs: CodeQL runs at 02:00, + # Python Compatibility at 04:00 and Vendor Update at 10:00. + - cron: '0 6 * * 6' + workflow_dispatch: + +permissions: + contents: read + +# Only pull-request runs may be superseded -- see the note in unit-tests.yml. A +# ref-keyed group drops a *pending* run when a newer one joins it, so scheduled +# and manual runs are keyed on the run id to give each one its own group. +concurrency: + group: lint-${{ github.event_name == 'pull_request' && github.ref || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + lint: + name: Lint + runs-on: ubuntu-latest + timeout-minutes: 30 + + steps: + - uses: actions/checkout@v7 + + # One interpreter, not a matrix. Unit Tests and Python Compatibility sweep + # 3.10-3.15 because they check that the package *runs* everywhere; these + # tools read the source, and running them five times over would produce + # five copies of the same findings for five times the runner cost. + # + # 3.14 specifically, matching cron-vendor.yml, because it is the newest + # non-experimental version in the test matrix and the version the recorded + # counts above were measured on -- so a count that moves means the code + # moved, not the interpreter. + - uses: actions/setup-python@v7 + with: + python-version: '3.14' + + # `.[all]` rather than a bare install: pylint and mypy resolve imports, so + # findings depend on what is importable. Note `pypcap` and `pypcapfile` are + # marked `python_version < '3.12'` in pyproject.toml and so are absent here + # by design -- that is the source of pylint's 8 `import-error` messages, + # and they will persist until those engines support a current Python. + - name: Install package and lint tools + run: | + set -x + + python -m pip install -U pip setuptools wheel + python -m pip install -e .[all] + python -m pip install -U vermin pylint mypy bandit + + # Every step below runs the *Makefile* target rather than spelling the + # flags out again, with `RUN=` emptying the `pipenv run` prefix the targets + # use locally. That is what keeps this workflow and a developer's `make + # pylint` running the same check -- restating the flag sets here would let + # the two drift the first time either changed, which is how "passes on my + # machine" starts. + # + # `continue-on-error: true` reports the step's real verdict as a non- + # blocking annotation and lets the following steps run, so one tool's + # findings never hide another's. The `status=$?` dance is needed because + # the default shell is `bash -e`: without it the summary lines after a + # non-zero tool would never execute. + + - name: bandit + continue-on-error: true + run: | + set +e + make bandit RUN= 2>&1 | tee bandit.log + status=${PIPESTATUS[0]} + set -e + + { + echo '### bandit' + echo '```' + sed -n '/^Run metrics:/,$p' bandit.log || echo 'no run metrics in output' + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + + exit $status + + - name: mypy + continue-on-error: true + run: | + set +e + make mypy RUN= 2>&1 | tee mypy.log + status=${PIPESTATUS[0]} + set -e + + { + echo '### mypy' + echo '```' + tail -n 1 mypy.log + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + + exit $status + + # `vermin-ci`, not `vermin`: the `vermin` target redirects into temp/ and + # hands the file to an editor, so its exit status is the editor's and a run + # that found violations still succeeds. Same flags, via $(VERMIN_FLAGS). + # + # This one needs a decision rather than a cleanup before it can block, and + # the decision is not CI's to make. vermin.ini sets `targets = 3.6` while + # the code's real floor is 3.11 -- `enum.StrEnum` and + # `enum.show_flag_values` are 3.11 members, and 86 modules import + # `typing_extensions`. pyproject.toml's `requires-python` still claims + # `>=3.6, <4`, so the declared floor and the actual one are five releases + # apart. Whichever way that is resolved -- raise `requires-python`, or + # raise `targets` to match it -- belongs in its own change. + - name: vermin + continue-on-error: true + run: | + set +e + make vermin-ci RUN= 2>&1 | tee vermin.log + status=${PIPESTATUS[0]} + set -e + + { + echo '### vermin' + echo '```' + grep -E '^(Minimum required versions|Incompatible versions|Target versions not met):' vermin.log \ + || echo 'no verdict in output' + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + + exit $status + + # Last because it is by far the slowest -- minutes over ~90k lines -- so + # the three quick verdicts reach the summary before it starts. + - name: pylint + continue-on-error: true + run: | + set +e + make pylint RUN= 2>&1 | tee pylint.log + status=${PIPESTATUS[0]} + set -e + + { + echo '### pylint' + echo '```' + printf 'messages by class (C/E/R/W):\n' + grep -oE ': [A-Z][0-9]{4}:' pylint.log | grep -oE '[A-Z]' | sort | uniq -c || echo 'none' + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + + exit $status diff --git a/Makefile b/Makefile index 3a71d89961..07437b154b 100644 --- a/Makefile +++ b/Makefile @@ -126,19 +126,46 @@ isort: pipenv run isort -l100 -ppcapkit pcapkit/{const,vendor}/*/*.py pipenv run isort -l100 -ppcapkit util/*.py examples/generators/*.py +# The command prefix that puts the lint tools on PATH. Locally that is pipenv, as +# everywhere else in this file. The lint workflow installs the tools into the +# job's own interpreter instead and overrides this to empty (`make pylint RUN=`), +# which is the whole point of the variable: CI runs these recipes rather than +# restating their flags, so there is exactly one definition of each tool's flag +# set and "clean locally, red in CI" cannot start from the two drifting apart. +RUN ?= pipenv run + +# Flag sets are variables rather than literals for the same reason -- `vermin` +# needs two recipes (see below) and would otherwise carry two copies of its +# flags. Note the `pcapkit` before the flags as well as after: that is how this +# recipe has always read, and vermin de-duplicates the paths (it reports 496 +# files analyzed either way), so it is preserved verbatim rather than tidied. +VERMIN_FLAGS = --backport argparse --backport enum --backport importlib --backport ipaddress --backport typing --backport typing_extensions --no-parse-comments --eval-annotations -vv +PYLINT_FLAGS = --load-plugins=pylint.extensions.check_elif,pylint.extensions.docstyle,pylint.extensions.emptystring,pylint.extensions.overlapping_exceptions --disable=all --enable=F,E,W,R,basic,classes,format,imports,refactoring,else_if_used,docstyle,compare-to-empty-string,overlapping-except --disable=blacklisted-name,invalid-name,missing-class-docstring,missing-function-docstring,missing-module-docstring,design,too-many-lines,eq-without-hash,old-division,no-absolute-import,input-builtin,too-many-nested-blocks,broad-except,singleton-comparison,ungrouped-imports --max-line-length=120 --init-import=yes +MYPY_FLAGS = --follow-imports=silent --ignore-missing-imports --show-column-numbers --show-error-codes +BANDIT_FLAGS = -r + vermin: mkdir -p temp - pipenv run vermin pcapkit --backport argparse --backport enum --backport importlib --backport ipaddress --backport typing --backport typing_extensions --no-parse-comments --eval-annotations -vv pcapkit > temp/vermin.txt + $(RUN) vermin pcapkit $(VERMIN_FLAGS) pcapkit > temp/vermin.txt command -v code >/dev/null && code temp/vermin.txt || cat temp/vermin.txt +# What CI runs. The `vermin` target above redirects into temp/ and then hands the +# file to an editor, so its exit status is the editor's (or `cat`'s) and a run +# that found violations still succeeds -- fine at a desk, useless as a check. +# This one writes to stdout and lets vermin's failure propagate, which is what +# `targets = 3.6` in vermin.ini is for: vermin exits non-zero when the target is +# not met, and the code's real floor is 3.11. +vermin-ci: + $(RUN) vermin pcapkit $(VERMIN_FLAGS) pcapkit + pylint: - pipenv run pylint --load-plugins=pylint.extensions.check_elif,pylint.extensions.docstyle,pylint.extensions.emptystring,pylint.extensions.overlapping_exceptions --disable=all --enable=F,E,W,R,basic,classes,format,imports,refactoring,else_if_used,docstyle,compare-to-empty-string,overlapping-except --disable=blacklisted-name,invalid-name,missing-class-docstring,missing-function-docstring,missing-module-docstring,design,too-many-lines,eq-without-hash,old-division,no-absolute-import,input-builtin,too-many-nested-blocks,broad-except,singleton-comparison,ungrouped-imports --max-line-length=120 --init-import=yes pcapkit + $(RUN) pylint $(PYLINT_FLAGS) pcapkit mypy: - pipenv run mypy --follow-imports=silent --ignore-missing-imports --show-column-numbers --show-error-codes pcapkit + $(RUN) mypy $(MYPY_FLAGS) pcapkit bandit: - pipenv run bandit -r pcapkit + $(RUN) bandit $(BANDIT_FLAGS) pcapkit profile: $(MAKE) -C test profile