Skip to content

ci: rebuild engine-tests as a Python × engine matrix with three cell outcomes - #849

Merged
JarryShaw merged 2 commits into
mainfrom
ci-845-engine-matrix-rebuild
Sep 27, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
ci-845-engine-matrix-rebuild

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • ci — workflows or build tooling

Description of your pull request and other information

Closes #845's remaining part — the matrix rebuild. The other two parts already landed (#846, and
promoting the engine checks to required in ruleset 23497679).

⚠️ This cannot merge before the ruleset is updated, or it blocks every PR

engine-tests's job name changes from Engines Python ${{ matrix.python-version }} to
Engines Python ${{ matrix.python-version }} (${{ matrix.engine }}). Ruleset 23497679 matches on
name, and it currently requires these five, which this PR stops producing:

Engines Python 3.10   Engines Python 3.11   Engines Python 3.12
Engines Python 3.13   Engines Python 3.14

A required check that never reports blocks merges permanently. So the ruleset's required list must
be changed in the same window as this merge — 22 contexts today, five of which become dead. The
new subset is the maintainer's call; the obvious candidate is the 3.10–3.14 cells excluding 3.15,
which is 30 of the 36 names. I have not touched the ruleset — that is yours.

The defect this fixes

Still live on main (30bca999c): pyproject.toml:201 declares
PyPCAPFile = [ "pypcapfile; python_version < '3.12'" ], so installing via the extra resolves to
nothing on 3.12/3.13/3.14 — yet Engines Python 3.12/3.13/3.14 passed and, since the earlier
promotion, gated merges while exercising an engine that was not installed.

The matrix, enumerated independently by me from the committed YAML

6 Python versions × 6 engines = 36 cells, 10 include overrides, tallying:

outcome cells which
supported 26 install via the pyproject extra; must genuinely pass
unsupported 6 PyPCAPFile 3.12/3.13/3.14/3.15, PyShark 3.14/3.15 — installed by raw pip install <dist> to bypass the marker; the engine's own PYTHON_CEILING decline is what the tests assert
not-installable 4 PyPCAP 3.12–3.15 — real C-extension build failure, logged via ::notice, test step skipped

The install step never passes silently — if a not-installable cell ever installs, or a
supported/unsupported cell ever fails to, that is a loud failure, not a quiet one. Distinguishing
a transient pip failure from the genuine compile failure a not-installable cell expects is still a
heuristic match against pip's captured output, so a false negative there is a loud false red
rather than a silent one — it still blocks every PR on that check, just honestly rather than
invisibly. (An earlier revision's transient-vs-compile grep was in fact backwards on real pip output —
an unanchored bare-number match fired on ordinary "(NNN kB)" download-size lines and on compiler
diagnostics quoting a line number, while matching none of the genuine compile-failure markers; fixed
to key on pip's own retry/network vocabulary instead, with the absence of a compile marker required
as a second check.) 3.15 is non-blocking via continue-on-error: ${{ matrix.python-version == '3.15' }} plus
allow-prereleases on setup-python. if: ${{ inputs.gate-only != true }} is retained, so the job
still reports on every PR. timeout-minutes drops 45 → 30.

test, integration and pypcap-parity job names are unchanged.

Gaps stated rather than buried

  • The per-engine split removes the old job's incidental shared venv, which gave the multi-engine
    HAS_RUNTIME gate (needs DPKT+Scapy+PyShark together) and the vendor extra's
    HAS_VENDOR_DEPS coverage as a side effect. Not restored here; worth its own issue. (Corrected
    from an earlier draft of this description: crypto and NGAP are not lost — the test job
    installs both and runs the whole unit tier regardless of this PR, and there is in fact no
    HAS_NGAP gate anywhere in tests/ — grep -rl '\bHAS_NGAP\b' tests/ returns nothing.)
  • Also newly dropped, and not part of the pre-existing HAS_RUNTIME/vendor gap above:
    engine-tests's Scapy cell now runs an explicit test-path list rather than the old job's
    whole-unit-tier selection, so it no longer reaches every HAS_SCAPY-gated test that whole-tier
    selection used to. tests/protocols/transport/test_sctp_unit.py's one method is restored — its
    path is now in the Scapy cell's TEST_PATHS, at zero cost, since that file carries no
    HAS_DPKT-gated test of its own. The other two, one method each in tests/interface/test_core.py
    and tests/interface/test_misc.py, are not: no per-PR job now runs those with Scapy installed —
    recorded in tests/_dependency_gates.py's HAS_SCAPY exclusion rather than restored, since both
    files also carry their own HAS_DPKT-gated tests and adding them to the Scapy cell without DPKT
    would trade one dark flag for another; worth its own review.
  • Integration-tier tests are not re-run per cell — they need fixture regeneration, which itself needs
    Scapy+DPKT together. Still covered in aggregate by integration and pypcap-parity.
  • Only a real CI run can confirm: that dpkt/scapy/pyshark have installable 3.15
    prereleases at all; that raw pip install pypcapfile succeeds on 3.12+; that raw
    pip install pypcap genuinely fails to build on 3.12+; the wall-clock cost of 36 legs vs 5; and
    that the apt installs work on the 3.15 runner image. GitHub Actions cannot be exercised locally.

On the six 3.15 contexts

Left out of any required-check recommendation, deliberately. python-compatibility.yml's own
comment records its 3.15 leg being moved to a schedule as non-gating queue contention, and that leg
was already continue-on-error — a precedent against running 3.15 per-PR, not for it, and that leg
cost ~14s where each of these six pays apt + pip + pytest. The ruleset update above should stay
scoped to the 3.10–3.14 cells.

@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) test Pull requests that add or correct tests (test: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES on c4b26ce2b — cross-review (opus, a different model from the sonnet author).
I re-derived the blocker myself rather than relaying it, and it is real.

A prose comment breaks five required checks. tests/_dependency_gates.py parses this workflow
as data, and tests/test_tier_guard.py asserts against it — unit-tier, so it runs in the test
job behind required checks Python 3.10…Python 3.14. Line 247 of the new YAML is a comment:

  # `pip install -e '.[...,PyPCAPFile,...]'` silently installed *nothing* for

_dependency_gates.py:1230 matches pip install -e '\.\[([^]]*)\]', and job_sections() slices job
bodies on ^ ([\w-]+):, so a comment preceding engine-tests: is attributed to integration.
Measured by pointing the repo's own guard at this head's YAML with tests/ unchanged:

AssertionError: the 'integration' job runs pytest but has 2 "pip install -e '.[...]'" lines,
                and this guard cannot tell which extras the run would have
-> 29 failed, 73 passed, 474 subtests in tests/test_tier_guard.py

So the ruleset window this PR already warns about is not sufficient — five more required checks
go red, from prose.

Required changes:

  1. Reword line 247 so it contains no literal pip install -e '.[…]', with a comment saying why.
  2. Teach tests/_dependency_gates.py the per-cell install and explicit-paths shape, and update the
    exclusion entries this PR falsifies (315, 337, 361–372, 381–382, 430, 461, 484–492, 529). This PR
    cannot be workflow-only.
  3. Undisclosed coverage drop: HAS_SCAPY unit-tier coverage in tests/interface/test_core.py,
    tests/interface/test_misc.py, tests/protocols/transport/test_sctp_unit.py disappears from every
    per-PR job — the old engine-tests was the only one installing Scapy and running the unit tier,
    which is exactly what the HAS_SCAPY exclusion at line 430 cites as its justification. Restore it
    or record the loss there.
  4. Harden the install step: set +e is set before the preamble, so pip install -U pip and
    pip install -e '.[test]' go unchecked — a broken base install on a not-installable cell reads as
    "expected" and the job goes green having installed and run nothing. Also, any nonzero pip exit
    counts as the expected compile failure, so a PyPI 503 passes; and a supported cell whose extra
    resolves to nothing still exits 0, which is ci: engines not exercised across their claimed Python ranges — PyShark has no real-capture coverage, PyPCAPFile dark on 3 of 5 legs #845's own finding 2 reproduced structurally.
  5. The description overstates two gaps: crypto and NGAP are not lost (test installs both and runs
    the whole unit tier; there is no HAS_NGAP gate in tests/ at all). Only vendor and HAS_RUNTIME are.
  6. python-compatibility.yml's own comment is a precedent against running 3.15 per-PR, not for it —
    it was moved to a schedule because it was non-gating queue contention, and it was already
    continue-on-error. That leg was ~14s; these six do apt + pip + pytest. Leave the six 3.15 contexts
    out of the ruleset.

Claims 1, 2, 5 and 7 hold exactly as written — the 36-cell / 26-6-4 tally was independently
re-derived, and the PyPCAPFile marker at pyproject.toml:201 confirmed verbatim on main.

Labelled review: needs-changes. Dispatching the fix; do not merge this yet.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 27, 2026
@JarryShaw
JarryShaw force-pushed the ci-845-engine-matrix-rebuild branch from c4b26ce to e38e849 Compare September 27, 2026 13:19
@JarryShaw

Copy link
Copy Markdown
Owner Author

All three blockers cleared plus the six follow-ups. Head c4b26ce2b → e38e849ee, so the previous
NEEDS CHANGES verdict is superseded — relabelled review: pending and a fresh cross-review is
dispatched on a different model from the one that did the fix.

Verified by me on a scratch tree with current main merged in (506b92c95, i.e. post-#850), not just
on the branch — the PR reads BEHIND, and a guard that models test paths can interact with a main that
has just gained new test files:

  • merge into current main is clean, exit 0, no conflicts
  • the acceptance test passes on the merged tree: tests/test_tier_guard.py → 102 passed, 558
    subtests
    , and plain unittest → Ran 102 tests … OK, so no pytest-subtests undercount
  • matrix unchanged: 36 cells, 10 include entries, 0 unmatched, tally 26 supported / 6
    unsupported / 4 not-installable
    ; job name, continue-on-error (3.15-only) and
    if: ${{ inputs.gate-only != true }} all as before
  • B1 closed: all 5 remaining pip install -e '.[...]' occurrences are real install lines — 0 in
    comments
  • item 5 closed, checked in the shell rather than taken on trust: set -eu now covers the preamble
    (pip install -U pip, .[test]) and set +e appears only immediately before the per-engine install;
    pip output is captured and grepped to separate a transient failure from a genuine compile failure;
    each engine carries an IMPORT_NAME for a post-install python -c "import …" so a supported cell
    whose extra resolved to nothing fails loudly; RAW_PKGS is an array, retiring the unquoted
    word-splitting

B2 was completed rather than deferred — tests/_dependency_gates.py gained a paths field and an
'explicit' selection kind, plus _engine_matrix_variants() yielding one Job per engine by parsing the
workflow's own EXTRA=/TEST_PATHS= case blocks, and seven exclusion entries were rewritten.
HAS_MYPY's now-false engine-tests dark entry was dropped.

B3 was recorded, not restored — the HAS_SCAPY unit-tier drop is now in the description's gap list and in
the HAS_SCAPY exclusion's own reason. The stated ground: each of those three files also carries a
HAS_DPKT gate, so adding them to the Scapy cell's TEST_PATHS alone would trade one dark flag for
another. I agree that is its own change, not a workflow-only fix.

Two things still true and unchanged by this fix. Ruleset 23497679 is untouched — 5 required contexts
still stop being produced, and per B1's finding that edit has to land with this PR, not before it. And 3
pre-existing mypy errors in the touched files (_dependency_gates.py:719,1035, test_tier_guard.py:436)
were confirmed outside the changed hunks — pre-existing, not introduced.

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES on e38e849ee — second cross-review (opus, a different model from the sonnet fixer),
run post-merge against current main. Three required changes, all three confirmed by me independently
rather than relayed. review: needs-changes applied.

1. B3's justification is false for one of the three files. Both the description and the HAS_SCAPY
exclusion reason say each of the three also carries HAS_DPKT-gated tests, so adding them to the Scapy
cell "would trade one dark flag for another". Measured:

tests/interface/test_core.py                  HAS_DPKT=2  HAS_SCAPY=2
tests/interface/test_misc.py                  HAS_DPKT=5  HAS_SCAPY=2
tests/protocols/transport/test_sctp_unit.py   HAS_DPKT=0  HAS_SCAPY=2   <- no trade exists

For test_sctp_unit.py there is no trade: adding it to the Scapy arm restores 1 of the 3 lost
HAS_SCAPY scopes at zero cost in new dark flags. Add that path, and correct the claim to "two of the
three" in both places.

2. The transient-vs-compile grep is backwards on real pip output, and would make three required cells
deterministically red.
unit-tests.yml:494's alternation (^|[^0-9])(50[0-9]|429)([^0-9]|$) is
unanchored. Run against realistic transcript lines:

MATCHES(transient)   Downloading pypcap-1.3.0.tar.gz (500 kB)
MATCHES(transient)   pcap.c:501:24: error: 'PyThreadState' has no member named 'use_tracing'
no match             error: command '/usr/bin/gcc' failed with exit code 1
no match             ERROR: Failed building wheel for pypcap

pip prints a (NNN kB) size line on essentially every sdist download, so PyPCAP × 3.12/3.13/3.14 —
three cells inside the set this PR recommends promoting to required — classify as transient and exit 1
permanently; re-running cannot clear a deterministic transcript. The genuine compile markers match
nothing. This also falsifies the description's "neither a silent green nor a silent red is possible": a
loud false red on a required check blocks merges just as effectively. Match pip's real transient markers
(HTTP error 5\d\d, status code 429, Retrying (Retry(, HTTPSConnectionPool) and/or require the
absence of a compile marker.

3. _engine_matrix_variants() never reads the matrix.engine list it keys on. It detects the shape on
the substring if 'matrix.engine' not in section and then builds its Job set exclusively from the shell
case arms via re.findall. Delete - PyShark from the engine: list — the obvious way to retire a cell —
and the guard still models 6 cells, still credits HAS_PYSHARK, and tests/test_tier_guard.py stays
green while CI runs 5. That is the silently-wrong-Job failure mode, in the same
comment-vs-regex family as the original blocker. Parse the engine: list and assert set-equality with the
case-arm keys. Note no test exercises any of the function's three raise AssertionError branches, in a
file whose own docstring says "the only honest way to show a guard works is to break the thing it guards
and watch it fire."

Everything else confirmed, including the merge being clean, 102 passed / 558 subtests with
Ran 102 tests … OK under plain unittest, the 36-cell 26/6/4 tally, B1 closed, set -eu covering the
preamble, the import check reaching every supported cell, and the HAS_MYPY drop being correct rather
than convenient. Two prose errors were also found and are worth fixing while you are in there: the
HAS_PYSHARK exclusion says "all 4" methods where it is 3 of 4, and unit-tests.yml:438-439 claims a
default -eo pipefail that GitHub does not give a Linux run: step.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 27, 2026
@JarryShaw
JarryShaw force-pushed the ci-845-engine-matrix-rebuild branch from e38e849 to 102ad32 Compare September 27, 2026 14:00
- Replace the single "install every engine, one leg per Python version"
  engine-tests job with a genuine two-dimension matrix: 6 Python versions
  (3.10-3.15) x 6 engines (DPKT, Scapy, PyShark, PyPCAPFile, PyPCAP,
  PCAP_CT), 36 cells, per the maintainer's ruling on #845. Each cell
  resolves to supported/unsupported/not-installable via a new `expect`
  field, closing the gap where PyPCAPFile's own pip marker silently
  installed nothing on 3.12-3.14 while the leg still reported green.
- Reword the install-step comment that broke five required unit-tier
  checks: it held a literal `pip install -e '.[...]'` that
  tests/_dependency_gates.py's job_sections() attributed to the wrong job.
- Teach tests/_dependency_gates.py the new per-(matrix.engine) shape: one
  Job per engine cell, an `explicit` selection over literal TEST_PATHS.
  `_engine_matrix_variants()` now parses the `engine:` matrix list itself
  and asserts set-equality against the `case "$ENGINE" in ...` arm keys,
  rather than trusting the arms alone -- deleting a cell from the matrix
  list used to leave it silently still modelled. Every regex in that
  function now runs against a comment-stripped copy of the section, so a
  commented-out arm placed after a live one cannot win the last-wins
  `dict()` merge. Update the DEPENDENCY_GATE_EXCLUSIONS entries whose
  justification the shape change falsified (HAS_PCAP_CT, HAS_PYPCAPFILE,
  HAS_VENDOR_DEPS, HAS_SCAPY, HAS_PYSHARK, HAS_RUNTIME, HAS_MYPY).
- Restore the one HAS_SCAPY path that costs nothing to restore
  (tests/protocols/transport/test_sctp_unit.py, which carries no HAS_DPKT
  gate of its own) into the Scapy cell's TEST_PATHS; record rather than
  restore the other two (test_core.py, test_misc.py), which do each carry
  their own HAS_DPKT gate and would trade one dark flag for another.
- Harden the install step: scope `set +e` to the engine install only (a
  broken baseline no longer reads as an expected not-installable cell);
  distinguish a transient pip failure from the genuine compile failure a
  not-installable cell expects by keying on pip's own retry/network
  vocabulary and requiring the absence of a compile marker, rather than an
  unanchored bare-number match that fired on ordinary "(NNN kB)"
  download-size lines and compiler diagnostics alike; add a post-install
  import assertion so a supported cell whose extra resolves to nothing
  fails loudly instead of skipping green; quote PCAP_CT's two-package raw
  install via an array instead of unquoted word-splitting.
- Fix two prose errors: the HAS_PYSHARK exclusion claimed its cell's
  TEST_PATHS covered all 4 gated methods (it is 3 of 4 -- the fourth is
  integration-tier and outside any engine-tests cell's reach); the
  install step's own comment claimed the shell's default `-eo pipefail`
  (GitHub's Linux `run:` default is plain `bash -e {0}`, no pipefail --
  harmless here since the step sets `set -eu` itself, but wrong).
- Correct the PR description's stated gaps (crypto/NGAP were never lost;
  only vendor, HAS_RUNTIME and the HAS_SCAPY paths above are), and its
  claim that a silent false red is impossible (a heuristic transient-vs-
  compile match can still misfire loudly, which blocks merges just as
  effectively as a silent one would).
- Add falsifiability tests for all four of _engine_matrix_variants's
  raise branches, including the exact matrix-list-vs-case-arm mismatch
  the second cross-review demonstrated, and a regression test for the
  commented-out-arm hazard above.

Build: not run (workflow-only change, GHA cannot be exercised locally).
YAML parses clean; 36 cells re-derived as 26 supported/6 unsupported/4
not-installable. tests/test_tier_guard.py: 107 passed, 558 subtests
passed (pytest); Ran 107 tests ... OK (plain unittest, same tree).
@JarryShaw
JarryShaw force-pushed the ci-845-engine-matrix-rebuild branch from 102ad32 to 0cc59b7 Compare September 27, 2026 14:04
@JarryShaw

Copy link
Copy Markdown
Owner Author

All three required changes cleared. Head e38e849ee → 0cc59b73c, so the needs-changes verdict is
superseded — relabelled review: pending, third cross-review dispatched. Verified by me on a scratch tree
with current main merged in (exit 0, clean):

Item 1 — the grep is now correct in both directions. The unanchored bare-number branch is gone, and
the classification requires a transient marker and the absence of a compile marker (&& !). Re-tested
against the exact lines that broke it:

t=0 compile=0 -> compile/other   Downloading pypcap-1.3.0.tar.gz (500 kB)
t=0 compile=0 -> compile/other   pcap.c:501:24: error: 'PyThreadState' has no member named …
t=1 compile=0 -> TRANSIENT       Retrying (Retry(total=4, connect=None)) after connection broken
t=1 compile=0 -> TRANSIENT       HTTP error 503 while getting https://pypi.org/simple/pypcap/
t=1 compile=0 -> TRANSIENT       HTTPSConnectionPool(host='pypi.org', port=443): Read timed out.
t=0 compile=1 -> compile/other   error: command '/usr/bin/gcc' failed with exit code 1
t=0 compile=1 -> compile/other   ERROR: Failed building wheel for pypcap

The false-positive pair no longer classifies as transient, real transient markers do, and a message carrying
both is treated as the compile failure it also is.

Item 2 — the parser now reads the engine: list. tests/_dependency_gates.py:1337 parses it and asserts
set-equality with the case-arm keys, and :1332 strips #-prefixed lines before every regex — closing the
"commented-out arm silently wins" hazard, which was the original blocker's class relocated into the new code.
Five falsifiability tests added, covering all three raise branches plus the commented-arm regression, so
the file's own standard — "the only honest way to show a guard works is to break the thing it guards and
watch it fire"
— now holds for this function too. tests/test_tier_guard.py goes 102 → 107 passed / 560
subtests
, re-derived as Ran 107 tests … OK under plain unittest.

Item 3 — tests/protocols/transport/test_sctp_unit.py added to the Scapy arm (unit-tests.yml:563), the
zero-cost restore. The HAS_SCAPY exclusion and the description are rescoped to "two of the three".

Matrix unperturbed by the TEST_PATHS change: 36 cells, 10 include entries, 0 unmatched, 26 supported /
6 unsupported / 4 not-installable
.

Both prose errors also fixed: HAS_PYSHARK corrected from "all 4" to "3 of 4", and the false claim about the
shell's default -eo pipefail (GitHub's Linux run: default is bash -e {0}, no pipefail).

Unchanged and still yours: ruleset 23497679 must be updated in the same window as this merge, because
five required contexts stop being produced. mypy shows the same 3 pre-existing errors with shifted line
numbers only.

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on 0cc59b73c — third cross-review (opus), run post-merge against f990f2149. Three rounds
of NEEDS CHANGES, and this one found nothing warranting a fourth.

I re-derived the claim this PR was caught for twice — that a falsifiability test might pass on both sides.
Swapped e38e849ee's _dependency_gates.py under the new tests and ran the five in isolation:

pre-fix:   3 failed, 2 passed
  FAILED test_deleting_an_engine_from_the_matrix_list_is_caught      (AssertionError not raised)
  FAILED test_a_test_paths_arm_with_no_matching_extra_arm_is_caught  (AssertionError not raised)
  FAILED test_a_commented_out_test_paths_arm_does_not_silently_win
post-fix:  5 passed

The two that pass pre-fix say so in their own docstrings — "the pre-existing base_installs branch,
exercised for the first time". Honest labelling, not a test claiming protection it does not provide.

What the review established beyond my floor, each measured rather than reasoned:

  • It captured a real pip install pypcap failure (pypcap-1.3.0 on CPython 3.14, throwaway venv) and ran
    the workflow's exact two-part grep against it plus four hand-built transcripts — including the
    retry-then-genuine-compile-failure case. No realistic loud false red on the three required PyPCAP
    cells, which was round 2's high-severity direction. The mechanism is visible in the real transcript:
    error: subprocess-exited-with-error appears in every modern-pip build failure, so the && !compile
    conjunct dominates a stray transient phrase.
  • Item 3's consequence, not just its presence: a fresh venv with only .[test] then .[Scapy] —
    dpkt/pyshark/pcapfile/pcap all confirmed absent — running the Scapy cell's exact command gave
    45 passed, 63 subtests, exit 0, with test_scapy_cross_check genuinely running rather than skipping.
    Its gate is HAS_RUNTIME, whose four members are pcapkit base dependencies. The "zero-cost restore" holds.
  • No fourth HAS_SCAPY file was overlooked — the only other non-integration one is
    tests/protocols/test_generated_pcap_runtime.py, which the old selection also excluded via
    --ignore-glob='*_runtime.py'.
  • Commenting out an engine: entry raises the same loud mismatch as deleting it — better than required.

Two non-blocking residuals recorded rather than folded away, both worth a comment if anyone touches this
again: subprocess-exited-with-error is not specifically a compile marker — pip emits it for any
build-pipeline subprocess failure, including a pure network failure inside build isolation, so such a
transcript reads as "unbuildable". Harm is bounded: a not-installable cell runs no tests either way, and the
case that matters — pypcap becoming buildable — is caught loudly by the status -eq 0 branch. Separately,
no matching distribution found sits in the transient list and is not transient; it would give a permanent
red if pypcap were yanked. Not live today.

This still cannot merge until ruleset 23497679 is updated in the same window — five required contexts
(Engines Python 3.10…3.14) stop being produced, and per round 1's B1 finding the ruleset edit alone was
never sufficient. The recommended required set is the 30 non-3.15 cell names; leave the six 3.15 contexts out.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

CI all passed but russet gates not getting their expected results? Actually are those ruleset things necessary? We can already gate on CI passing.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Recorded, verbatim: "CI all passed but russet gates not getting their expected results? Actually are those
ruleset things necessary? We can already gate on CI passing."

On the first half — yes, and here is exactly why. The five required contexts are name-specific and this PR
renames the job that produces them:

required by ruleset 23497679 but NOT produced by #849:
  Engines Python 3.10   Engines Python 3.11   Engines Python 3.12
  Engines Python 3.13   Engines Python 3.14

#849 emits Engines Python 3.10 (DPKT) … 36 of them. It produces 61 check runs against 22 required
contexts, and those five will never arrive. It currently reads BEHIND rather than BLOCKED only because
strict_required_status_checks_policy: true makes staleness the first complaint; bring it up to date and the
five missing contexts become the block, permanently.

On the second half — measured, and the answer is no: there is no separate CI gate. required_status_checks
is it.

gh api repos/JarryShaw/PyPCAPKit/branches/main/protection
  -> 404  "Branch not protected"

Classic branch protection is off, so ruleset 23497679 is the only enforcement on main. And the other
rules do not gate on CI or review:

pull_request: required_approving_review_count: 0, require_last_push_approval: false,
              dismiss_stale_reviews_on_push: false,
              required_review_thread_resolution: true, require_extra_approval_for_unattributed_changes: true
deletion, non_fast_forward: branch safety only

So removing the required-checks list would make a red PR mergeable — GitHub would still display the
failures but nothing would block the button. That is the opposite of what you want, particularly with agents
raising the PRs.

But your instinct is right in a different form, and the repo already has the mechanism. 22 name-specific
contexts are brittle — this PR is the second time a rename has silently orphaned one. The standard fix is to
require one aggregate context whose job needs: all the others, instead of enumerating them. You already
have gate — "Gate (full suite, Python 3.14)" — but it is if: ${{ inputs.gate-only == true }}, so it never
runs on a pull request.

Two ways forward, and this one is a judgement call rather than a measurement:

  1. Short term, to land ci: rebuild engine-tests as a Python × engine matrix with three cell outcomes #849: swap the five stale names for the 30 non-3.15 cell names. Brittle, but it is a
    one-line ruleset edit and unblocks today.
  2. Better, and worth its own issue: make a per-PR aggregate job that needs: every gating job and fails if
    any did, then require that single context. Rulesets do not support wildcards in check contexts, so an
    aggregate job is the only way to stop the required list drifting every time the matrix changes.

Say which and I will do it — (1) is yours to apply since the ruleset is yours; (2) I can file and implement.

@JarryShaw JarryShaw added the needs: decision Waiting on the maintainer to decide — not blocked by other work label Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Take the better path (2)

@JarryShaw

Copy link
Copy Markdown
Owner Author

And am I good to merge this one for now?

@JarryShaw

Copy link
Copy Markdown
Owner Author

Recorded, verbatim: "Take the better path (2)" — the per-PR aggregate job. Mine to implement; a worker is on
it now, and this head will move.

For the record, the measurement that makes (2) the better path. The ruleset (23497679) requires 22 check
contexts by exact name, and rulesets do not support wildcards:

Python 3.10-3.14                      (5)   <- the `test` job, still real
Integration Python 3.10-3.14          (5)   <- the `integration` job, still real
Compat Python 3.10-3.14               (5)   <- NO job emits these. Stale.
Engines Python 3.10-3.14              (5)   <- #849 replaces these with 36 matrix cells
PyPCAP/PyPCAPFile parity 3.10-3.11    (2)   <- the `pypcap-parity` job, still real

Option (1) would have meant listing 30 cell names by hand and re-listing them on every matrix change, forever.
(2) adds one job that needs: every per-PR gating job, so the ruleset requires one context and never needs
touching again.

Two traps the worker is briefed on, because getting them wrong makes the gate worse than no gate:

  • A plain needs: job is skipped when a dependency fails, and a skipped required check can register as
    satisfied. So the aggregate runs if: always() and explicitly fails on any needs.*.result != 'success',
    rather than relying on needs: alone.
  • It must not needs: gate. That job is release-only (if: inputs.gate-only == true) and never runs per PR, so
    depending on it would leave the aggregate permanently skipped.

Your remaining step, once this lands: replace all 22 contexts in the ruleset with the single new one. I will
report its exact name.

@JarryShaw JarryShaw removed the needs: decision Waiting on the maintainer to decide — not blocked by other work label Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

And am I good to merge this one for now?

Yes. 0cc59b73c is verified and unchanged: 58 CheckRuns pass, 0 fail, 0 incomplete, MERGEABLE, and it
carries a GOOD TO GO from a third opus cross-review on exactly this sha. Nothing is in flight against it — I
have redirected the aggregate-job worker off this branch to a separate PR based on main, precisely so it
does not hold this one up. The head will not move.

One thing to expect when you click merge: the ruleset will likely still complain, and not because of this PR.
Five of its 22 required contexts — Compat Python 3.10 through 3.14 — are emitted by no job in the
workflow
, and that was already true before this branch existed. They can never be satisfied, so the gate is
unsatisfiable today regardless of what this PR does. You will need to bypass, which is your call as owner.

That is the whole reason (2) is worth building rather than patching the list: after the aggregate PR lands you
replace all 22 contexts with one name that never changes again. I will give you the exact string when it is
up.

Ordering note, since you have asked about it before: #657 and #719 still merge last, after everything else is
closed. This one is not part of that constraint.

@JarryShaw
JarryShaw merged commit b918c86 into main Sep 27, 2026
61 checks passed
@JarryShaw
JarryShaw deleted the ci-845-engine-matrix-rebuild branch September 27, 2026 15:20
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Correction to what I told you above, and it matters before you touch the ruleset.

I said five of the 22 required contexts — Compat Python 3.10 through 3.14 — are "emitted by no job in the
workflow" and that the gate is "unsatisfiable today". That is false. I scanned only
.github/workflows/unit-tests.yml. Those contexts are emitted by a different workflow file:

.github/workflows/python-compatibility.yml:31   name: Compat Python ${{ matrix.python-version }}
                                          :4,6  on: push / pull_request

And they pass on every PR — measured on #856 just now:

Compat Python 3.10  pass  13s      Compat Python 3.13  pass  14s
Compat Python 3.11  pass  13s      Compat Python 3.14  pass  14s
Compat Python 3.12  pass  15s      Compat Python 3.15 (scheduled)  skipping

So of the 22, only the five Engines Python 3.10–3.14 are genuinely stale — the ones #849 replaced with
36 matrix cells. My "the gate is unsatisfiable regardless of any PR" claim was wrong too; the failure you saw was
those five Engines names alone.

The consequence for the ruleset edit: do NOT replace all 22 contexts with the single new one. A needs:
cannot reach a job in another workflow file, so the aggregate covers 17 of 22 — the five Python, five
Integration, five stale Engines and two parity names. The five Compat names must be retained
alongside Required checks passed, or Python 3.10–3.14 import coverage silently stops gating.

I have held #856 at review: needs-changes for exactly this — its own comment carries my wrong classification
and instructs the reader to drop the five Compat names. Nothing to do on your side; #849 itself is unaffected.

JarryShaw added a commit that referenced this pull request Sep 27, 2026
Ruleset 23497679's required_status_checks names 22 exact check contexts by
literal string, and GitHub rulesets support no wildcard. Of those 22, only
the five `Engines Python <version>` names are actually dead -- `engine-tests`
stopped emitting them once it became a Python x engine matrix. The five
`Compat Python <version>` names are live, emitted by the `compatibility` job
in python-compatibility.yml, and must stay required as-is; nothing in this
file can stand in for a job in a different workflow file.

- Add a `required-checks` job that `needs: [test, integration, engine-tests,
  pypcap-parity]` and reports one stable check context, `Required checks
  passed`, that the ruleset can require in place of the 17 `test`/
  `integration`/`Engines`/`pypcap-parity` slots (not the five `Compat` ones).
- Guard it with `if: ${{ always() && inputs.gate-only != true }}`: without
  `always()`, a plain `needs:` job is *skipped* -- not failed -- the moment
  any dependency fails, and GitHub's own docs list a skip as a passing
  status for required checks, which would make this gate worse than none.
  `gate-only != true` keeps it from spuriously failing on the release-only
  `gate-only: true` call path, where its four dependencies are themselves
  skipped by design.
- The job's own step inspects each dependency's `needs.<job>.result`
  explicitly, printing every value and failing loudly (naming the job) on
  anything other than `success`.
- Comment documents one open, unverified risk: whether a `continue-on-error`
  3.15 `engine-tests` cell's real failure is neutralised to `success` in
  `needs.engine-tests.result` the way it is for the run's own conclusion is
  not stated in GitHub's docs. Flagged rather than resolved by splitting the
  3.15 legs into their own job, which was judged too much churn for a job
  #849 only just rebuilt, to close a single undocumented edge.

YAML verified with `yaml.safe_load` via the repo venv; `git diff --numstat`
against `origin/main` shows one file, 140 insertions, 0 deletions.
JarryShaw added a commit that referenced this pull request Sep 27, 2026
engine-tests ran one Python-version leg per interpreter regardless of
which extraction engines that interpreter could actually install, so
pyproject.toml's own PyPCAPFile marker resolved to nothing on 3.12-3.14,
yet those legs still passed and, once promoted to a required check, gated
merges while exercising an engine that was never installed.

Rebuilt as a genuine Python x engine matrix -- 36 cells after 10 include
overrides -- with three honest outcomes: 26 supported cells that must
genuinely pass, 6 unsupported cells that assert the engine's own decline,
and 4 not-installable cells where a real C-extension build failure is
logged and the test step skipped. Job names change shape, which needed
the branch-protection ruleset's required-check list updated in the same
window.
JarryShaw added a commit that referenced this pull request Sep 27, 2026
Adds util/pyshark_encap_map.py, a generator that regenerates
ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in
place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built
stop being hand-maintained. Guards against its own worst failure mode: a
naive LinkType(dlt) lookup cannot detect an unmapped DLT, because
_missing_ mints a placeholder rather than raising, so the generator
snapshots every known value before any lookup.

The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is
version-pinned -- editcap -T accepts 226 encapsulations on Wireshark
4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run
only under the measured version. One line of pyshark.py itself changes as
a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical
name #848 gave value 209.

util/changelog_md.py regenerated CHANGELOG.md for all seven entries added
across this and the six preceding commits (#838, #846, #847, #848, #849,
#850, #853); `--check` exit 0.
JarryShaw added a commit that referenced this pull request Sep 27, 2026
#846 and #850

- #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified"
  measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an
  unmodified process(), but its FLAG-bound range has no unassigned gap, so
  its _missing_ carries no bounded-range extend_enum branch to fix. The
  real discriminator is that branch, not the process() override; reworded,
  and the "other 84" split into the 83 that do override process() and the
  one that doesn't but has nothing to fix either. Re-derived independently
  against 5e25db2^..5e25db2 (not main, which would fold in #847's own
  edit to ipx/socket.py): 22 registries have no process() override, 21 of
  them have the bounded-range branch, matching
  tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES
  (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining
  the exclusion).

- #846's "Only tests/integration/test_engine_runtime.py and
  test_engine_parity.py change" is false -- 30bca99's own numstat also
  touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only
  these two test files change".

- #850 gets the breaking marker: its own prose already says the fallback
  is gone and an unrecognised encapsulation or filter name now raises
  MissingKeyError instead of substituting a plausible DLT -- the same
  shape as the two existing markers at AppType.get (1.5.0.rst:2456) and
  the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to
  lead with the marker and the subject, matching their wording and
  placement; the later restatement of the same fact is dropped.

None of these are code or test changes -- prose only, matching the
cross-review's own framing. #848 stays unmarked (its own "Not breaking"
paragraph holds); #838, #849 and #853 stay unmarked per explicit
instruction not to add markers beyond what was asked.
JarryShaw added a commit that referenced this pull request Sep 27, 2026
…ck job

Ruleset 23497679 names 22 exact check contexts with no wildcard support,
so the required list needs hand-editing on every matrix change; five of
them (the old single-cell Engines Python <version> names) are already
dead since #849, and the five Compat Python 3.10-3.14 names stay live and
untouched.

Adds one job, required-checks (context "Required checks passed"),
depending on test/integration/engine-tests/pypcap-parity with
if: always() so it cannot itself be silently skipped -- the failure mode
GitHub's own troubleshooting docs warn a bare needs: list falls into --
plus an explicit per-dependency check that names which job broke. Not
breaking: workflow-only, no library or test code changes. The 17-of-22
ruleset replacement it enables is a separate repository setting this PR
does not itself touch.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
engine-tests ran one Python-version leg per interpreter regardless of
which extraction engines that interpreter could actually install, so
pyproject.toml's own PyPCAPFile marker resolved to nothing on 3.12-3.14,
yet those legs still passed and, once promoted to a required check, gated
merges while exercising an engine that was never installed.

Rebuilt as a genuine Python x engine matrix -- 36 cells after 10 include
overrides -- with three honest outcomes: 26 supported cells that must
genuinely pass, 6 unsupported cells that assert the engine's own decline,
and 4 not-installable cells where a real C-extension build failure is
logged and the test step skipped. Job names change shape, which needed
the branch-protection ruleset's required-check list updated in the same
window.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
Adds util/pyshark_encap_map.py, a generator that regenerates
ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in
place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built
stop being hand-maintained. Guards against its own worst failure mode: a
naive LinkType(dlt) lookup cannot detect an unmapped DLT, because
_missing_ mints a placeholder rather than raising, so the generator
snapshots every known value before any lookup.

The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is
version-pinned -- editcap -T accepts 226 encapsulations on Wireshark
4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run
only under the measured version. One line of pyshark.py itself changes as
a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical
name #848 gave value 209.

util/changelog_md.py regenerated CHANGELOG.md for all seven entries added
across this and the six preceding commits (#838, #846, #847, #848, #849,
#850, #853); `--check` exit 0.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
#846 and #850

- #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified"
  measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an
  unmodified process(), but its FLAG-bound range has no unassigned gap, so
  its _missing_ carries no bounded-range extend_enum branch to fix. The
  real discriminator is that branch, not the process() override; reworded,
  and the "other 84" split into the 83 that do override process() and the
  one that doesn't but has nothing to fix either. Re-derived independently
  against 5e25db2^..5e25db2 (not main, which would fold in #847's own
  edit to ipx/socket.py): 22 registries have no process() override, 21 of
  them have the bounded-range branch, matching
  tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES
  (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining
  the exclusion).

- #846's "Only tests/integration/test_engine_runtime.py and
  test_engine_parity.py change" is false -- 30bca99's own numstat also
  touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only
  these two test files change".

- #850 gets the breaking marker: its own prose already says the fallback
  is gone and an unrecognised encapsulation or filter name now raises
  MissingKeyError instead of substituting a plausible DLT -- the same
  shape as the two existing markers at AppType.get (1.5.0.rst:2456) and
  the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to
  lead with the marker and the subject, matching their wording and
  placement; the later restatement of the same fact is dropped.

None of these are code or test changes -- prose only, matching the
cross-review's own framing. #848 stays unmarked (its own "Not breaking"
paragraph holds); #838, #849 and #853 stay unmarked per explicit
instruction not to add markers beyond what was asked.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
…ck job

Ruleset 23497679 names 22 exact check contexts with no wildcard support,
so the required list needs hand-editing on every matrix change; five of
them (the old single-cell Engines Python <version> names) are already
dead since #849, and the five Compat Python 3.10-3.14 names stay live and
untouched.

Adds one job, required-checks (context "Required checks passed"),
depending on test/integration/engine-tests/pypcap-parity with
if: always() so it cannot itself be silently skipped -- the failure mode
GitHub's own troubleshooting docs warn a bare needs: list falls into --
plus an explicit per-dependency check that names which job broke. Not
breaking: workflow-only, no library or test code changes. The 17-of-22
ruleset replacement it enables is a separate repository setting this PR
does not itself touch.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) ci Pull requests that change CI or workflow configuration (ci: subject prefix) test Pull requests that add or correct tests (test: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ci: engines not exercised across their claimed Python ranges — PyShark has no real-capture coverage, PyPCAPFile dark on 3 of 5 legs

1 participant