Skip to content

ci: wire the existing linters into CI as advisory checks - #626

Merged
JarryShaw merged 4 commits into
mainfrom
ci/lint-workflow
Sep 22, 2026
Merged

JarryShaw merged 4 commits into
mainfrom
ci/lint-workflow

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

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 4ecac90, which established
that the local linters are not a CI gate. No issue tracks it.

Measured first, then designed

Every tool was run against this branch's tree at the flag sets the Makefile already uses, on the
repo venv's Python 3.14.7. Counts as of 15189abff:

linter result exit recommendation
bandit 8 findings — 7 medium, 1 low, 0 high 1 advisory, promote first
mypy 115 errors in 39 files (496 checked) 1 advisory
vermin 105 files flagged; needs 3.11, vermin.ini targets 3.6 1 advisory, blocked on a decision
pylint 5902 messages — 80 E, 4767 W, 519 R, 541 C 30 advisory, not promotable at this rule set

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 is
continue-on-error: true and writes its count to the run summary instead; promoting one is
deleting that one line.

Promotion order, recorded in the workflow header:

  • bandit is nearest. 7 of its 8 are B104 "binding to all interfaces" on
    address: ... = '0.0.0.0' default arguments in the 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 of the count.
  • vermin needs a decision rather than a cleanup; see below.
  • pylint is not realistically promotable here: 4358 of its 4767 warnings are
    unused-argument (2931), redefined-builtin (808) and super-init-not-called (619), all three
    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.

Shape and cost

One new workflow rather than a job bolted onto an existing one, following CodeQL's precedent that
static analysis stands on its own — adding it to unit-tests.yml would couple linting to that
workflow's workflow_call gate and let a lint finding block cron-vendor's version bump.

One job, one interpreter. The tools read the source, so a matrix would produce five copies of
the same findings, and four separate jobs would pay for four checkouts and four .[all] installs
to run four cheap tools. Python 3.14 matches cron-vendor.yml and is the version the counts above
came from, so a count that moves means the code moved rather than the interpreter.

Triggers are pull_request, a Saturday 06:00 schedule, and workflow_dispatch — deliberately
not push: [main]
, which the other validation workflows 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, adds 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. 06:00 Saturday is an hour clear of CodeQL (02:00), Python
Compatibility (04:00) and Vendor Update (10:00).

Local and CI run the same flags, by construction

The four flag sets became Makefile variables and pipenv run became $(RUN), so the workflow
runs make pylint RUN= rather than restating the flags. That leaves exactly one definition of
each flag set, so the two cannot drift into "clean on my machine, red in CI". Verified the
expanded commands are byte-identical to the literals they replace (make -n pylint RUN= diffed
against the previous recipe string). vermin.ini is picked up from the repository root in both
places.

vermin needed a second recipe: the existing 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. The
new vermin-ci writes to stdout and lets the failure propagate, off the same $(VERMIN_FLAGS).
The original vermin target is behaviourally unchanged.

No linter configuration was weakened to make anything pass. Nothing was narrowed, no rule
disabled, no severity floor raised.

Found and deliberately not fixed

Neither belongs in a CI wiring change, and pcapkit/** is untouched by this PR.

  1. The declared Python floor and the real one are five releases apart. vermin puts the code's
    minimum at 3.11 — enum.StrEnum and enum.show_flag_values are 3.11 members, and 86
    modules import typing_extensions — while vermin.ini sets targets = 3.6 and
    pyproject.toml still declares requires-python = ">=3.6, <4". That mismatch is exactly why
    vermin exits non-zero, so it is the thing standing between vermin and a blocking job. Resolving
    it either way (raise requires-python, or raise targets to match it) is a user-visible
    packaging decision and wants its own change.
  2. Three names used in string annotations are never imported, which mypy flags as
    name-defined: Optional at pcapkit/protocols/schema/internet/ipv6_route.py:271, Protocol
    at the same file's :136, and Any at pcapkit/utilities/logging.py:350. Latent rather than
    live, because cast()'s first argument and a TYPE_CHECKING-guarded __init__ stub's
    annotations are strings Python never evaluates — but they would break any runtime annotation
    evaluation, and ipv6_route.py:271 sits two lines from an unreachable finding in the same
    function.

Verification

  • make -n for all four targets in both forms; CI-form expansions diffed byte-for-byte against
    the recipes they replace.
  • make bandit RUN= and make vermin-ci RUN= run end-to-end with the tools on PATH: 8 findings
    and 105 files respectively, matching the direct runs, each exiting non-zero, and vermin-ci
    confirmed not to write temp/vermin.txt.
  • All four step-summary snippets executed against the real captured logs; output checked.
  • Workflow YAML parsed, and its trigger/step/continue-on-error structure asserted.
  • python util/changelog_md.py --check exits 0.

No changelog entry: CI and developer tooling, not user-visible. No tests run — nothing here
touches the package.

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 4ecac90, 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 15189ab 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.
@JarryShaw
JarryShaw merged commit 9846458 into main Sep 22, 2026
26 checks passed
@JarryShaw
JarryShaw deleted the ci/lint-workflow branch September 22, 2026 13:06
JarryShaw added a commit that referenced this pull request Sep 22, 2026
…ed (#656)

The *Coding style* section still told contributors that none of the linters is
run by the pull-request workflows, which #626 made false when it added
`.github/workflows/lint.yml`. Saying only that they now run would mislead in the
other direction, so the replacement leads with the part that decides what a
contributor should expect: all four steps carry `continue-on-error: true`, so a
red linter cannot fail a pull request. It also keeps `isort` out of that group --
isort appears in `cron-vendor.yml` as a formatter over the regenerated
constants, not as a check -- and records the `make vermin` trap, whose exit
status is the viewer's rather than vermin's, which is why `make vermin-ci` is
the target CI runs.

Three smaller corrections found while checking the rest of the file against the
tree:

* The README has no *Testing* section any more. Its only mention of testing is
  a row in the *Documentation* table linking to `docs/source/testing.rst`,
  which is where the test commands moved.
* The `Changelog drift` job's triggers are scoped to `main` -- `push` and
  `pull_request` both carry `branches: [main]` -- rather than firing on every
  push and pull request in the repository.
* "The exception stops at the root" overstated the Markdown boundary: the three
  issue and pull-request templates under `.github/` are Markdown for the same
  GitHub-rendering reason the root files are.

No changelog entry, deliberately: none of this is user-visible, and a bullet
here would collide with the changelog consolidation currently in flight.
@JarryShaw JarryShaw added the ci Pull requests that change CI or workflow configuration (ci: subject prefix) label Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant