scapy: load the layer registry, so the engine actually dissects by JarryShaw · Pull Request #409 · JarryShaw/PyPCAPKit

JarryShaw · 2026-09-16T04:16:38Z

Closes #406.

The defect

engine='scapy' returned every frame as Raw unless the calling program happened to have imported scapy.all itself — so the engine delivered none of the dissection it exists for, silently.

scapy/__init__.py is inert, so from scapy import sendrecv left conf.l2types with 0 entries and DLT 1 → None. PcapReader then could not map plain Ethernet.

	fresh process	caller did `import scapy.all`
before	`Raw`×6, 0 streams	`Ether`×6, 3 streams
after	`Ether`×6, 3 streams	`Ether`×6, 3 streams

That invariance to ambient import state is the fix — the test-order dependence was a symptom.

The mechanism that hid it, worth recording: pcapkit/toolkit/scapy.py's lazy from scapy.layers.inet6 import IPv6ExtHdrFragment repairs the registry mid-run — measured, l2types 0 → 14 and DLT 1 → Ether — but one call after sniff() has already decided how to parse. Now documented there so it isn't mistaken for the mechanism again.

`scapy.all` chosen on correctness, not cost

The narrow alternative — importing only the modules that register a DLT — fixes the link layer and then silently truncates payloads on 6 of the 15 sample captures:

dhcp.pcapng, dhcp_{big,little}_endian, test.pcapng, profile.pcapng
    narrow: Ethernet / IP / UDP / Raw      broad: … / UDP / BOOTP / DHCP options
test.pcap
    narrow: Ethernet / IP / UDP / Raw      broad: … / UDP / DNS

That is precisely the "looks fixed and isn't" outcome — worse than the status quo, because it would pass a link-layer test. And it is unfixable by enumeration: it needs every payload binding, i.e. conf.load_layers, which scapy maintains and a hardcoded list here would silently rot against.

There is also no cheaper complete option: scapy.layers.all, the bare registry populator, measures 0.459s against scapy.all's 0.46s. The layer loading is the cost.

Import costs, fresh process, 5 runs: sendrecv 0.135s · narrow-minimal 0.176s · narrow-complete 0.21s · scapy.all 0.46s.

The import stays lazy inside the engine, and that was verified rather than assumed: import pcapkit is unchanged at 0.51s with scapy.all absent from sys.modules. Only engine construction moves, 0.108s → 0.433s, once per process, paid solely by callers who ask for this engine.

The warning is deliberately not suppressed

The CryptographyDeprecationWarning comes from scapy.layers.dcerpc pulling in the TLS layer, so any complete load emits it — not scapy.all specifically — and it subclasses UserWarning, not DeprecationWarning, so default filters do show it.

Left alone because pcapkit/utilities/warnings.py states the house position that filters belong to the consumer, and silencing it would hide the only notice that a future cryptography has broken scapy's TLS layer. The one-line filter to quiet it is documented in the module docstring instead.

Three tests encoded the wrong rationale

Each corrected and justified from the capture rather than re-baselined:

test_scapy_engine_finds_no_streams_for_this_capture → test_scapy_engine_matches_the_default_engine, asserting three-axis parity, with assertNotIsInstance(frame[0], Raw) first so a regression names the cause instead of surfacing as 0 != 3.
tests/integration/test_engine_runtime.py asserted 'Raw' where the two tests immediately above it report the same frame as Ethernet:IPv6:IPv6_ICMP (default) and Ethernet:IP6:ICMP6 (dpkt).
tests/integration/test_engine_parity.py — comment only; its byte-level assertions pass unchanged, since bytes() is identical either way.

New tests/foundation/engines/test_scapy_engine.py asserts in a subprocess — the only place ambient pollution cannot fool it. Verified to fail on the unfixed engine with scapy's conf.l2types was empty.

Sibling engines checked — scapy was the only one broken

engine	finding
`dpkt`	builds its registry in the package `__init__`, which Python runs before any submodule — narrow and full imports byte-identical (74 modules, `_typesw`=11 both). Verified empirically
`pyshark`	no registry at all; already gated by `unsupported_reason()`
`pypcapfile`	eagerly imports all four submodules it uses — the opposite mistake
`pypcap`	single extension module
`pcap_ct`	runs `probe()` before `import pcap`

The last three are reasoned from source, since those libraries are not installed here.

`unsupported_reason()` deliberately says nothing

A non-None return swaps the engine for the default. scapy is installed, importable and fully working — the bug was ours. Using the hook as a notice board would silently discard the user's choice.

Verification

tier	baseline	after
fixture-free CI	647 passed, 5 skipped	649 passed, 5 skipped (+2, the new tests)
`tests/integration`	1 failed, 84 passed, 2 skipped	identical
`_runtime` / `_regression`	48 passed, 10 skipped	identical

Order dependence gone: tests/toolkit then test_misc.py is now 52 passed / 4 skipped where the baseline was 1 failed / 49 passed (3 != 0); the reversed order is identical.

A note on that baseline: my first attempt measured a /tmp copy with no .git, which made the tier guard skip 18 tests and produced a bogus number. Discarded and re-measured in the real worktree with samples present.

One pre-existing failure, not from this change

test_pyshark_engine_currently_breaks_on_modern_asyncio fails identically on unmodified main: AssertionError: AttributeError not raised, because PyShark.unsupported_reason() — added in #405 — now intercepts on 3.14 and falls back with an EngineWarning. #405 made that test obsolete and did not update it. Worth its own issue; folding it in here would conflate two defects.

tests/toolkit/test_scapy_unit.py still leaks scapy state into conf, which purge_modules(['pcapkit']) cannot undo. It no longer matters, and I deliberately did not chase it: the leak is legitimate (those tests need the classes for fixtures) and the right fix was removing the dependence on ambient state, which is what landed.

`engine='scapy'` returned every frame as `Raw` unless the calling program happened to have imported `scapy.all` itself, so the engine delivered none of the dissection it exists to provide -- silently. `scapy/__init__.py` is inert, so `from scapy import sendrecv` left `conf.l2types` with **0 entries** and `DLT 1 -> None`, and `PcapReader` could not map plain Ethernet. before, fresh process Raw x6, 0 TCP streams before, after import scapy.all Ether x6, 3 streams after, either way Ether x6, 3 streams That invariance to ambient import state *is* the fix; the test-order dependence was a symptom of it. The mechanism that hid this is worth recording: `pcapkit/toolkit/scapy.py`'s lazy `from scapy.layers.inet6 import IPv6ExtHdrFragment` **repairs** the registry mid-run -- measured, `l2types` 0 -> 14 and `DLT 1 -> Ether` -- but one call after `sniff()` has already decided how to parse. Documented there so it is not mistaken for the mechanism again. `scapy.all` chosen on correctness, not cost. The narrow alternative -- importing only the modules that register a DLT -- fixes the *link* layer and then silently truncates *payloads* on 6 of the 15 sample captures: dhcp.pcapng, dhcp_{big,little}_endian, test.pcapng, profile.pcapng narrow: Ethernet/IP/UDP/Raw broad: .../UDP/BOOTP/DHCP options test.pcap narrow: Ethernet/IP/UDP/Raw broad: .../UDP/DNS which is the "looks fixed and isn't" outcome, and it is unfixable by enumeration: it needs every payload binding, i.e. `conf.load_layers`, which scapy maintains and a hardcoded list here would silently rot against. There is also no cheaper complete option -- `scapy.layers.all`, the bare registry populator, measures 0.459s against `scapy.all`'s 0.46s, so the layer loading *is* the cost. The import stays lazy inside the engine, and that was verified rather than assumed: `import pcapkit` is unchanged at 0.51s with `scapy.all` absent from `sys.modules`; only engine construction moves, 0.108s -> 0.433s, once per process, paid solely by callers who ask for this engine. The `CryptographyDeprecationWarning` is deliberately not suppressed. It comes from `scapy.layers.dcerpc` pulling in the TLS layer, so *any* complete load emits it -- not `scapy.all` specifically -- and it subclasses `UserWarning`, so default filters show it. `pcapkit/utilities/warnings.py` states the house position that filters belong to the consumer, and silencing it here would hide the only notice that a future `cryptography` has broken scapy's TLS layer. The one-line filter is documented instead. Three tests encoded the wrong rationale and are corrected, each justified from the capture rather than re-baselined. `test_scapy_engine_finds_no_streams_for_this_capture` became `test_scapy_engine_matches_the_default_engine`, asserting three-axis parity with `assertNotIsInstance(frame[0], Raw)` **first**, so a regression names the cause instead of surfacing as `0 != 3`. `tests/integration/test_engine_runtime.py` asserted `'Raw'` where the two tests above it report the same frame as `Ethernet:IPv6:IPv6_ICMP` and `Ethernet:IP6:ICMP6`. New `tests/foundation/engines/test_scapy_engine.py` asserts in a **subprocess**, the only place ambient pollution cannot fool it; it fails on the unfixed engine with `scapy's conf.l2types was empty`. Sibling engines checked, and scapy was the only one broken: `dpkt` builds its registry in the package `__init__`, so narrow and full imports are byte-identical (74 modules, `_typesw`=11 both); `pyshark` has no registry and is already gated by `unsupported_reason()`; `pypcapfile` eagerly imports all four submodules it uses; `pypcap` is a single extension module; `pcap_ct` probes before importing. `unsupported_reason()` deliberately says nothing here -- a non-`None` return *swaps the engine for the default*, and scapy is installed, importable and fully working. The bug was ours. Verified: fixture-free CI 649 passed / 5 skipped against a 647/5 baseline, the delta being the two new tests; integration and the runtime/regression tiers byte-identical to baseline. Order dependence gone -- `tests/toolkit` then `test_misc.py` now 52 passed / 4 skipped where the baseline was 1 failed / 49 passed, and the reversed order is identical.

Copilot

🟢 Approval recommended

The engine change is targeted and well-covered by new/updated tests (including subprocess isolation) that directly prevent regression to undissected Raw frames.

Pull request overview

This PR fixes the Scapy extraction engine so it reliably performs protocol dissection in a fresh process by ensuring Scapy’s layer registries are populated by the engine itself (independent of any caller-side import scapy.all), and updates/adds tests to prevent regressions.

Changes:

Update pcapkit.foundation.engines.scapy.Scapy to import scapy.all during engine construction so sniff()/PcapReader can map DLTs and payload bindings.
Correct and strengthen integration/unit tests to assert dissected Scapy frames (e.g., Ether instead of Raw) and parity with the default engine.
Document the “late registry population” pitfall in pcapkit.toolkit.scapy to avoid future confusion about what those lazy imports accomplish.

File summaries

File	Description
pcapkit/foundation/engines/scapy.py	Import `scapy.all` in `Scapy.__init__` so the engine populates Scapy registries before `sniff()` dissects frames.
pcapkit/toolkit/scapy.py	Add documentation clarifying that lazy layer imports here are not the mechanism that makes engine dissection work.
tests/interface/test_misc.py	Fix Scapy expectations to require parity with the default engine and add a direct guard against `Raw` frames.
tests/integration/test_engine_runtime.py	Update runtime expectation from `Raw` to `Ether` and assert Scapy toolkit chain output.
tests/integration/test_engine_parity.py	Update commentary to reflect correct Scapy behavior while keeping byte-parity assertions unchanged.
tests/foundation/engines/test_scapy_engine.py	Add subprocess-based unit tests to validate the engine populates Scapy registries in an unpolluted interpreter.

Review details

Files reviewed: 6/6 changed files
Comments generated: 0
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The house convention copies prose out of a module docstring into its `.rst` rather than pulling it in with `automodule`, which means a docstring edit does not follow into the page. A lot of source changed recently, so a lot of pages had drifted. This is the sweep for contradictions -- a page naming a parameter that does not exist, a default that changed, a limitation since fixed -- not for thinness. The one that was actively breaking the build: `engines/index.rst` declared `.. _libpcap:` **twice**, the only duplicate explicit target in the tree, so all three ```libpcap`_`` links were dead and the build emitted four errors. The C library reference now uses the page's own ``:manpage:`libpcap(3)``` idiom and the PyPI target is kept. Documentation for work that had none: `Engine.unsupported_reason` (new in #396, undocumented on `engines/engine.rst` while two other pages already cross-referenced it), the whole `_pcap_backend` module including `Probe` -- noted as a Mapping rather than a tuple, since it is an `Info` subclass now -- and `PyPCAPFile.PYTHON_CEILING`. The Scapy section still described the pre-#409 `scapy.sendrecv` import and omitted the `CryptographyDeprecationWarning` note that its docstring gained. Examples that could not have worked as written, all measured: `extract(strict=True)` raises `TypeError` -- the argument is `reasm_strict`; `python -m pypcapkit` has no such module, only the PyPI *name* is `pypcapkit` and the runnable one is `pcapkit`; three CLI transcripts relied on extension autocorrect that is gated behind `-a`, so they raised `FileNotFound`; and "export to a JSON file with no format specified" actually writes a *tree* dump into `out.json`, because `format=None` defaults to `'tree'`. The RFC 815 walkthrough on `reassembly/tcp.rst` had drifted from the implementation in six separate ways -- an exclusive `last` where the code is inclusive (`first + len - 1`), `ISN <- DSN` where a SYN spends a sequence number (`PSN = DSN + 1 if SYN`), an unconditional hole update the code guards with `if info.len > 0`, comparison operators that do not match, a `more_fragments` flag TCP does not have, and a four-tuple BUFID in the wrong order. And `reassembly/ip/ipv6.rst` still documented the reassembly key as the **flow label**, which is exactly the defect fixed in `3642dcaa9` -- the label is optional and routinely zero, so keying on it collapsed distinct datagrams. Nine docstrings were fixed rather than their pages, in the cases where the page was right and the docstring named something that does not exist -- `pcapkit.dumper.*`, `pcapkit.traceflow`, `pcapkit.protocols.null`, `Extrator`, `tractflow`, `TOS*` for `ToS*`, and `ipv6_opts.py` declaring itself as `hopopt`. Confined to that case deliberately, so the sweep does not leave a page and its docstring newly inconsistent by its own hand. Verified against a pristine baseline built from `git archive HEAD` in its own venv, after discarding a first attempt that had overlapped with in-flight edits: **278 warning lines and 4 errors before, 274 and 1 after** -- zero new, four eliminated, all four the `libpcap` family. The surviving error is pre-existing and untouched. All 2671 autodoc targets across all 128 pages resolve, and the rendered HTML was checked rather than assumed: `unsupported_reason`, `Probe`, `PCAP_CT.backend`, `PYTHON_CEILING` and `Backend Detection` all appear, and `index.html` now emits four working libpcap(3) links where all three were broken.

* docs: fix 42 places where a page contradicts the code The house convention copies prose out of a module docstring into its `.rst` rather than pulling it in with `automodule`, which means a docstring edit does not follow into the page. A lot of source changed recently, so a lot of pages had drifted. This is the sweep for contradictions -- a page naming a parameter that does not exist, a default that changed, a limitation since fixed -- not for thinness. The one that was actively breaking the build: `engines/index.rst` declared `.. _libpcap:` **twice**, the only duplicate explicit target in the tree, so all three ```libpcap`_`` links were dead and the build emitted four errors. The C library reference now uses the page's own ``:manpage:`libpcap(3)``` idiom and the PyPI target is kept. Documentation for work that had none: `Engine.unsupported_reason` (new in #396, undocumented on `engines/engine.rst` while two other pages already cross-referenced it), the whole `_pcap_backend` module including `Probe` -- noted as a Mapping rather than a tuple, since it is an `Info` subclass now -- and `PyPCAPFile.PYTHON_CEILING`. The Scapy section still described the pre-#409 `scapy.sendrecv` import and omitted the `CryptographyDeprecationWarning` note that its docstring gained. Examples that could not have worked as written, all measured: `extract(strict=True)` raises `TypeError` -- the argument is `reasm_strict`; `python -m pypcapkit` has no such module, only the PyPI *name* is `pypcapkit` and the runnable one is `pcapkit`; three CLI transcripts relied on extension autocorrect that is gated behind `-a`, so they raised `FileNotFound`; and "export to a JSON file with no format specified" actually writes a *tree* dump into `out.json`, because `format=None` defaults to `'tree'`. The RFC 815 walkthrough on `reassembly/tcp.rst` had drifted from the implementation in six separate ways -- an exclusive `last` where the code is inclusive (`first + len - 1`), `ISN <- DSN` where a SYN spends a sequence number (`PSN = DSN + 1 if SYN`), an unconditional hole update the code guards with `if info.len > 0`, comparison operators that do not match, a `more_fragments` flag TCP does not have, and a four-tuple BUFID in the wrong order. And `reassembly/ip/ipv6.rst` still documented the reassembly key as the **flow label**, which is exactly the defect fixed in `3642dcaa9` -- the label is optional and routinely zero, so keying on it collapsed distinct datagrams. Nine docstrings were fixed rather than their pages, in the cases where the page was right and the docstring named something that does not exist -- `pcapkit.dumper.*`, `pcapkit.traceflow`, `pcapkit.protocols.null`, `Extrator`, `tractflow`, `TOS*` for `ToS*`, and `ipv6_opts.py` declaring itself as `hopopt`. Confined to that case deliberately, so the sweep does not leave a page and its docstring newly inconsistent by its own hand. Verified against a pristine baseline built from `git archive HEAD` in its own venv, after discarding a first attempt that had overlapped with in-flight edits: **278 warning lines and 4 errors before, 274 and 1 after** -- zero new, four eliminated, all four the `libpcap` family. The surviving error is pre-existing and untouched. All 2671 autodoc targets across all 128 pages resolve, and the rendered HTML was checked rather than assumed: `unsupported_reason`, `Probe`, `PCAP_CT.backend`, `PYTHON_CEILING` and `Backend Detection` all appear, and `index.html` now emits four working libpcap(3) links where all three were broken. * docs: align three field names with the code, drop two duplicate automethods Following the maintainer's decision that the octet tables exist to present the header structure as the RFCs define it, not to mirror the library's API -- but that the field names in them should still match what the code actually exposes. `` `tcp.opt` `` -> `` `tcp.options` `` (`data/transport/tcp.py:100`, `options: 'OrderedMultiDict[OptionNumber, Option]'`; nothing named `opt` exists) and `` `l2tp.ver` `` -> `` `l2tp.version` `` (`data/link/l2tp.py:44`, `version: 'int'`). The `tcp.options` cell needed re-padding afterwards, since the longer name pushed the description out of its column. `tcp.flags.ns` is a different case and stays. The bit *is* read off the wire -- `schema/transport/tcp.py:58` declares `ns: int` in the flags bitfield -- but the data model and the read site have been commented out in lockstep since `2e39aeb99` ("minor revision for TCP on typings", 2023-06-29): data/transport/tcp.py:52 #ns: 'bool' protocols/transport/tcp.py:447 #ns=bool(schema.offset['ns']), Left commented, deliberately: :rfc:`3540` was reclassified as Historic, and reviving a field nothing consumes adds surface for no gain. The table row is kept because the bit belongs to the header as the RFC defines it -- but it now carries a footnote saying the library does not surface it, so a reader does not go looking for `tcp.flags.ns` and find nothing. `mh.rst` listed `_read_opt_pad` and `_make_opt_pad` **twice each**, which is where two of the tree's duplicate-object warnings came from. Verified there is no `padn` sibling that the second line was meant to be -- Pad1 and PadN deliberately share one handler -- so they were simply stray copies. * docs: correct the reassembly snippets' attribute paths, and flag #415 Both Copilot findings on #413 hold up. The IPv4 and IPv6 reassembly glossary snippets named attributes that do not exist: - `ipv4.header` is not an attribute at all (`hasattr(IPv4, 'header')` is False, and `header` is not a field of the IPv4 data model). `toolkit/pcap.py` uses `ipv4.packet.header` and `bytearray(ipv4.packet.payload)`. - The IPv6 snippet's `ipv6.header` / `ipv6.payload` are really `ipv6_info.fragment.header` / `bytearray(ipv6_info.fragment.payload)`. Both snippets also elided the `.info` hop and wrote `tuple(a, b, c, d)`, which is not how `tuple` is called. They now mirror `toolkit/pcap.py` name for name, with a lead-in saying which object is which. The second finding's other half turned out to be a library defect rather than a comment error: `ihl` and `header` on this path *do* include the Fragment header, because `ipv6.py:341` adds each extension header's length before the Fragment-header check breaks the loop. `dpkt` and `scapy` exclude it and report 40 where this reports 48 for the same packet, and RFC 8200 s4.5 says the Fragment header is absent from a reassembled packet. Filed as #415; the doc now states what the code does today and warns that the value is not comparable across engines, rather than asserting semantics that are about to change. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: JarryShaw <15666417+JarryShaw@users.noreply.github.com>

JarryShaw requested a lite review from Copilot September 16, 2026 05:00

Copilot started reviewing on behalf of JarryShaw September 16, 2026 05:00 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

JarryShaw merged commit 615f89c into main Sep 16, 2026
25 checks passed

JarryShaw mentioned this pull request Sep 16, 2026

interface: add the three missing engine constants, and guard dpkt/scapy tracing #412

Merged

JarryShaw deleted the fix/scapy-layer-registry branch September 17, 2026 01:07

JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Sep 22, 2026

Navigation Menu

Uh oh!

scapy: load the layer registry, so the engine actually dissects - #409

scapy: load the layer registry, so the engine actually dissects#409
JarryShaw merged 1 commit into
mainfrom
fix/scapy-layer-registry

JarryShaw commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

JarryShaw commented Sep 16, 2026

The defect

scapy.all chosen on correctness, not cost

The warning is deliberately not suppressed

Three tests encoded the wrong rationale

Sibling engines checked — scapy was the only one broken

unsupported_reason() deliberately says nothing

Verification

One pre-existing failure, not from this change

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`scapy.all` chosen on correctness, not cost

`unsupported_reason()` deliberately says nothing