From eaa0c6f0558e5bb52f33a53f18983e4831050f35 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 15 Sep 2026 19:32:13 -0400 Subject: [PATCH 1/3] interface: dispatch follow_tcp_stream on the engine, and fix the dpkt crash `follow_tcp_stream` chose its reassembly adapter with `extraction.engine == 'dpkt'`, but `Extractor.engine` returns the `Engine` *instance*, so both branches were dead code, `fallback` was always true, and `pcapkit.toolkit.pcap`'s adapter was used against frames from whichever engine actually ran. The two `# type: ignore[comparison-overlap]` comments were mypy reporting exactly this and being silenced; both are gone, and mypy is clean on the file without them. It was only "clean" before because the always-false comparison made the `count=` call statically unreachable. Dispatch is now on the engine *type*. Chosen over `.name` because for a third-party engine registered as `class Foo(Engine, name='foo')` the registry key and the `.name` property diverge, and because a third-party engine subclassing a built-in then routes to the right adapter. An engine matching none of the built-ins warns and returns `()` rather than silently using the wrong one, which is the failure this issue is about. `fallback` became `pass_count`, set per adapter rather than per branch: the pcapkit-native adapters read the frame number off the dissected frame and take no `count`, while the third-party ones have no intrinsic number and must be passed `count=index`. The old code had those backwards. Second defect, and the issue's own diagnosis had it wrong -- I filed that issue, so the correction is mine. The dpkt `AttributeError: 'dict' object has no attribute 'packet'` does not come from the reassembly adapter at all; it fires in the PCAP trace-flow dumper at `pcapkit/dumpkit/pcap.py:139`, because dpkt and scapy hand the tracer plain dicts and `follow_tcp_stream` lets `trace_format` default to `'pcap'`. Handled here by upgrading to `'json'` for those engines -- silently when unset, with a `FormatWarning` when `'pcap'` was asked for explicitly. `extraction.py`'s guard is deliberately left alone: its own NOTE records that dpkt and scapy share the defect and were out of scope, so direct `Extractor` use still wants that guard extended separately. `engine='scapy'` returning 0 streams is not a bug: scapy does not recognise `in.pcap`'s link-layer type and yields all six frames as opaque `Raw`, so no frame carries TCP. The test asserts that *cause*, so a future scapy that learns the link type fails loudly rather than silently returning 0. The old tests mocked `engine` as a string, which is precisely why they never caught this, and that mocking cannot survive type-based dispatch -- so they are rewritten against real extraction of the committed `in.pcap`, with per-engine expected counts and the dpkt/scapy cases gated on the optional extras. Verified: dpkt goes from crashing to 3 streams, byte-identical to `default` including per-stream packet counts. tests/interface 12 passed; fixture-free unit tier 577 passed, 17 skipped against a 572/17 baseline -- the delta is exactly the new tests. --- pcapkit/interface/misc.py | 87 +++++++++-- tests/interface/test_misc.py | 290 +++++++++++++++++++++-------------- 2 files changed, 245 insertions(+), 132 deletions(-) diff --git a/pcapkit/interface/misc.py b/pcapkit/interface/misc.py index f827fad7e7..a7f9bbb577 100644 --- a/pcapkit/interface/misc.py +++ b/pcapkit/interface/misc.py @@ -10,20 +10,31 @@ """ import sys -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast from pcapkit.corekit.infoclass import Info, info_final +from pcapkit.foundation.engines.dpkt import DPKT as DPKT_Engine +from pcapkit.foundation.engines.pcap import PCAP as PCAP_Engine +from pcapkit.foundation.engines.pcapng import PCAPNG as PCAPNG_Engine +from pcapkit.foundation.engines.pypcapfile import PyPCAPFile as PyPCAPFile_Engine +from pcapkit.foundation.engines.scapy import Scapy as Scapy_Engine from pcapkit.foundation.extraction import Extractor from pcapkit.foundation.reassembly.tcp import TCP as TCP_Reassembly from pcapkit.utilities.exceptions import stacklevel -from pcapkit.utilities.warnings import EngineWarning, warn +from pcapkit.utilities.warnings import EngineWarning, FormatWarning, warn if TYPE_CHECKING: - from typing import Optional + from typing import Callable, Optional from typing_extensions import Literal from pcapkit.foundation.extraction import Packet + from pcapkit.foundation.reassembly.data.tcp import Packet as TCP_Data + + #: A toolkit's ``tcp_reassembly`` adapter. The pcapkit-native adapters take only + #: the frame; the third-party ones also accept a ``count`` keyword -- hence the + #: open ``...`` parameter list, which lets both call shapes type-check. + ReassemblyAdapter = Callable[..., 'Optional[TCP_Data]'] ByteOrder = Literal['little', 'big'] Formats = Literal['pcap', 'json', 'tree', 'plist'] @@ -83,20 +94,70 @@ def follow_tcp_stream(fin: 'Optional[str]' = None, verbose: 'bool' = False, EngineWarning, stacklevel=stacklevel()) engine = None + # NOTE: the DPKT and Scapy engines hand their frames to the flow tracer as plain + # :obj:`dict`\\ s, which the PCAP trace dumper cannot re-serialise -- it reaches + # for ``frame.packet`` and dies with ``AttributeError: 'dict' object has no + # attribute 'packet'`` (GH-399). The tracer defaults an unset ``format`` to + # ``'pcap'``, so following a stream through either engine crashes *during + # extraction*, before the reassembly below ever runs. :class:`Extractor + # ` already substitutes a dict-capable + # format for the PyShark and PyPCAPFile engines but deliberately leaves DPKT and + # Scapy out (see the note at its ``trace`` setup); apply the same remedy here so + # the stream is followed rather than crashed on. A caller that never asked for a + # trace format (``None``) is quietly upgraded; an explicit but unusable one is + # replaced with a warning, since it is a request that cannot be honoured. + if engine is not None and engine.lower() in ('dpkt', 'scapy') and format in ('pcap', 'cap', None): + if format is not None: + warn(f"extraction engine {engine} cannot write '{format}' trace files; " + "using 'json' instead", FormatWarning, stacklevel=stacklevel()) + format = 'json' + extraction = Extractor(fin=fin, fout=None, format=None, auto=True, extension=extension, store=True, files=False, nofile=True, verbose=verbose, engine=engine, layer=None, protocol=None, ip=False, ipv4=False, ipv6=False, tcp=True, reassembly=False, trace=True, trace_fout=fout, trace_format=format, trace_byteorder=byteorder, trace_nanosecond=nanosecond) # type: ignore[var-annotated] - fallback = False - if extraction.engine == 'dpkt': # type: ignore[comparison-overlap] - from pcapkit.toolkit.dpkt import tcp_reassembly # pylint: disable=import-outside-toplevel - elif extraction.engine == 'scapy': # type: ignore[comparison-overlap] - from pcapkit.toolkit.scapy import tcp_reassembly # isort: skip # pylint: disable=import-outside-toplevel + # NOTE: ``Extractor.engine`` returns the running engine *instance* (see + # :meth:`Extractor.engine `), + # never its name -- so the historical ``extraction.engine == 'dpkt'`` compared an + # object against a string and was *always* :data:`False`. Every capture then fell + # through to the pcapkit adapter, which crashed on DPKT frames and silently + # returned no streams on Scapy frames (GH-399). Dispatch on the engine *type* + # instead, and via :func:`isinstance` so that a third-party engine subclassing a + # built-in still reaches the adapter that matches its frames. + # + # The adapter and its call convention are chosen together: the pcapkit-native + # adapters (:mod:`~pcapkit.toolkit.pcap` and :mod:`~pcapkit.toolkit.pcapng`) read + # the frame number straight off the dissected frame and accept no ``count``, + # whereas the third-party adapters have no such number and must be handed the + # frame index as ``count`` -- so the two forms are not interchangeable. + exeng = extraction.engine + tcp_reassembly = None # type: Optional[ReassemblyAdapter] + pass_count = True + if isinstance(exeng, PCAP_Engine): + from pcapkit.toolkit import pcap as tk_pcap # isort: skip # pylint: disable=import-outside-toplevel + tcp_reassembly, pass_count = cast('ReassemblyAdapter', tk_pcap.tcp_reassembly), False + elif isinstance(exeng, PCAPNG_Engine): + from pcapkit.toolkit import pcapng as tk_pcapng # isort: skip # pylint: disable=import-outside-toplevel + tcp_reassembly, pass_count = cast('ReassemblyAdapter', tk_pcapng.tcp_reassembly), False + elif isinstance(exeng, DPKT_Engine): + from pcapkit.toolkit import dpkt as tk_dpkt # isort: skip # pylint: disable=import-outside-toplevel + tcp_reassembly = cast('ReassemblyAdapter', tk_dpkt.tcp_reassembly) + elif isinstance(exeng, Scapy_Engine): + from pcapkit.toolkit import scapy as tk_scapy # isort: skip # pylint: disable=import-outside-toplevel + tcp_reassembly = cast('ReassemblyAdapter', tk_scapy.tcp_reassembly) + elif isinstance(exeng, PyPCAPFile_Engine): + from pcapkit.toolkit import pypcapfile as tk_pypcapfile # isort: skip # pylint: disable=import-outside-toplevel + tcp_reassembly = cast('ReassemblyAdapter', tk_pypcapfile.tcp_reassembly) else: - from pcapkit.toolkit.pcap import tcp_reassembly # type: ignore[assignment] # isort: skip # pylint: disable=import-outside-toplevel - fallback = True + # A third-party engine pcapkit ships no reassembly adapter for. Falling back + # to the pcapkit adapter is exactly the GH-399 failure mode -- a wrong or empty + # result indistinguishable from a real one -- so warn and return no streams + # rather than reassemble frames whose shape we cannot parse. + warn(f'unsupported extraction engine for TCP stream following: {exeng.name}; ' + 'returning no streams', EngineWarning, stacklevel=stacklevel()) + return () streams = [] # type: list[Stream] frames = extraction.frame @@ -108,10 +169,10 @@ def follow_tcp_stream(fin: 'Optional[str]' = None, verbose: 'bool' = False, frame = frames[index-1] packets.append(frame) - if fallback: - data = tcp_reassembly(frame) - else: + if pass_count: data = tcp_reassembly(frame, count=index) + else: + data = tcp_reassembly(frame) if data is not None: reassembly(data) diff --git a/tests/interface/test_misc.py b/tests/interface/test_misc.py index 60c9b9adbe..ba19ae868c 100644 --- a/tests/interface/test_misc.py +++ b/tests/interface/test_misc.py @@ -1,131 +1,183 @@ from __future__ import annotations -import pathlib -import sys +import importlib.util +import tempfile import types import unittest +import warnings from unittest import mock -from tests._support import ROOT, ensure_package, load_module, purge_modules +from tests._support import purge_modules, sample_path + +#: Packages :mod:`pcapkit` needs before it can parse anything at all; the default +#: engine and therefore every test here depends on them. They are core install +#: dependencies, so this gate only ever skips on a deliberately minimal build. +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) +#: Whether the optional DPKT / Scapy engines can be selected. Both are optional +#: extras (``pypcapkit[DPKT]`` / ``[Scapy]``), absent from a plain ``[test]`` +#: install, so the engine-specific cases skip rather than fail on a fresh clone. +HAS_DPKT = importlib.util.find_spec('dpkt') is not None +HAS_SCAPY = importlib.util.find_spec('scapy') is not None + +#: TCP conversations the default (pcapkit-native) engine finds in the committed +#: ``in.pcap`` capture. Measured against ``examples/captures/in.pcap``, and the +#: yardstick every other engine is judged by -- see the class docstring for why +#: the per-engine expected values are *not* uniformly this number. +IN_PCAP_TCP_STREAMS = 3 + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class FollowTCPStreamTests(unittest.TestCase): + """:func:`~pcapkit.interface.misc.follow_tcp_stream` per extraction engine. + + ``examples/captures/in.pcap`` (committed, so unit-tier safe) holds three TCP + conversations. GH-399: the reassembly adapter was chosen by comparing the + engine *instance* to a string -- a comparison that is never true -- so every + engine silently used the pcapkit adapter regardless of which engine ran. That + crashed on DPKT frames (``AttributeError: 'dict' object has no attribute + 'packet'``) and returned an empty, misleading result on Scapy frames. + + The expected stream count is asserted per engine rather than as one shared + number, because engines with genuine dissection gaps legitimately differ: + DPKT dissects this capture fully and must match the default engine, whereas + Scapy cannot read its link layer at all and correctly finds nothing. + """ - -class InterfaceMiscTests(unittest.TestCase): def setUp(self) -> None: purge_modules(['pcapkit']) - - def _load_module(self, *, extractor_engine: str = 'default', - stream_indexes: tuple[int, ...] = (2, 1)): - ensure_package('pcapkit', ROOT / 'pcapkit') - ensure_package('pcapkit.foundation', ROOT / 'pcapkit' / 'foundation') - ensure_package('pcapkit.foundation.reassembly', - ROOT / 'pcapkit' / 'foundation' / 'reassembly') - ensure_package('pcapkit.toolkit', ROOT / 'pcapkit' / 'toolkit') - - calls = { - 'pcap': [], - 'dpkt': [], - 'scapy': [], - 'extractor': [], - } - frames = [ - types.SimpleNamespace(index=1, payload=b'one'), - types.SimpleNamespace(index=2, payload=b'two'), - ] - - extraction_module = types.ModuleType('pcapkit.foundation.extraction') - - class Extractor: - def __init__(self, **kwargs): - calls['extractor'].append(kwargs) - self.engine = extractor_engine - self.frame = frames - self.trace = types.SimpleNamespace(tcp=[ - types.SimpleNamespace(index=stream_indexes, fpout='stream.bin'), - ]) - - extraction_module.Extractor = Extractor - sys.modules['pcapkit.foundation.extraction'] = extraction_module - - tcp_module = types.ModuleType('pcapkit.foundation.reassembly.tcp') - - class TCP: - def __init__(self, strict=False): - self.strict = strict - self.datagram = [] - - def __call__(self, packet): - self.datagram.append(types.SimpleNamespace(index=packet.index, - payload=packet.payload)) - - tcp_module.TCP = TCP - sys.modules['pcapkit.foundation.reassembly.tcp'] = tcp_module - - def install_toolkit(name: str, *, uses_count: bool) -> None: - toolkit_module = types.ModuleType(f'pcapkit.toolkit.{name}') - - def tcp_reassembly(frame, *, count=None): - calls[name].append((frame, count)) - if frame.payload == b'two' and count == 2: - return None - index = count if uses_count else frame.index - return types.SimpleNamespace(index=index, payload=frame.payload) - - toolkit_module.tcp_reassembly = tcp_reassembly - sys.modules[f'pcapkit.toolkit.{name}'] = toolkit_module - - install_toolkit('pcap', uses_count=False) - install_toolkit('dpkt', uses_count=True) - install_toolkit('scapy', uses_count=True) - - module = load_module('pcapkit.interface.misc', 'pcapkit/interface/misc.py') - return module, calls - - def test_follow_tcp_stream_falls_back_for_pyshark_and_uses_pcap_helper(self) -> None: - module, calls = self._load_module(extractor_engine='default') - - with mock.patch.object(module, 'warn') as warn: - streams = module.follow_tcp_stream( - fin='in.pcap', - verbose=True, - extension=False, - engine='pyshark', - fout='trace.pcap', - format='pcap', - byteorder='little', - nanosecond=True, - ) - - warn.assert_called_once() - self.assertEqual(calls['extractor'][0]['engine'], None) - self.assertTrue(calls['extractor'][0]['trace']) - self.assertTrue(calls['extractor'][0]['tcp']) - self.assertEqual(calls['extractor'][0]['trace_fout'], 'trace.pcap') - self.assertEqual(calls['extractor'][0]['trace_format'], 'pcap') - self.assertEqual(len(calls['pcap']), 2) - self.assertEqual(calls['pcap'][0][1], None) - self.assertEqual(streams[0].filename, 'stream.bin') - self.assertEqual(streams[0].packets[0].payload, b'two') - self.assertEqual(streams[0].conversations, (b'one', b'two')) - - def test_follow_tcp_stream_uses_dpkt_and_scapy_counted_helpers(self) -> None: - module, calls = self._load_module(extractor_engine='dpkt') - streams = module.follow_tcp_stream(engine='dpkt') - self.assertEqual(calls['extractor'][0]['engine'], 'dpkt') - self.assertEqual(calls['dpkt'], [ - (streams[0].packets[0], 2), - (streams[0].packets[1], 1), - ]) - self.assertEqual(streams[0].conversations, (b'one',)) - - purge_modules(['pcapkit']) - module, calls = self._load_module(extractor_engine='scapy') - streams = module.follow_tcp_stream(engine='scapy') - self.assertEqual(calls['extractor'][0]['engine'], 'scapy') - self.assertEqual(calls['scapy'], [ - (streams[0].packets[0], 2), - (streams[0].packets[1], 1), - ]) - self.assertEqual(streams[0].conversations, (b'one',)) + # follow_tcp_stream always drives the flow tracer, whose output root + # defaults to './tmp' under the working directory when ``fout`` is unset + # (TraceFlow.__init__). Point it at a scratch directory so a test run + # writes nothing into the tree. + tmp = tempfile.TemporaryDirectory(prefix='pcapkit-follow-') + self.addCleanup(tmp.cleanup) + self.tmp_dir = tmp.name + + def _follow(self, **kwargs: object) -> tuple: + from pcapkit.interface.misc import follow_tcp_stream + + kwargs.setdefault('fout', self.tmp_dir) + return follow_tcp_stream(fin=sample_path('in.pcap'), **kwargs) # type: ignore[arg-type] + + def test_default_engine_finds_every_tcp_stream(self) -> None: + streams = self._follow() + self.assertEqual(len(streams), IN_PCAP_TCP_STREAMS) + # Each detected flow carries the frames traceflow grouped into it, and names + # the file its trace was written to. in.pcap's three TCP flows are one frame + # each and single-segment, so there is no multi-segment payload to + # reassemble -- the empty conversations are a property of this capture, not + # of the reassembly, which is why the parity test below compares them rather + # than requiring them non-empty. + for stream in streams: + self.assertGreaterEqual(len(stream.packets), 1) + self.assertIsNotNone(stream.filename) + + @unittest.skipUnless(HAS_DPKT, 'dpkt not installed') + def test_dpkt_engine_matches_the_default_engine(self) -> None: + # DPKT dissects in.pcap's Ethernet/IP/TCP in full, so it must agree with the + # native engine on both the stream count and the reassembled bytes. This is + # the regression guard for GH-399: before the fix the DPKT frame reached the + # pcapkit adapter and raised AttributeError, and the counted-vs-uncounted + # call convention is what makes the reassembled numbering line up. + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + native = self._follow() + foreign = self._follow(engine='dpkt') + + self.assertEqual(len(foreign), IN_PCAP_TCP_STREAMS) + self.assertEqual([len(stream.packets) for stream in foreign], + [len(stream.packets) for stream in native]) + self.assertEqual([stream.conversations for stream in foreign], + [stream.conversations for stream in native]) + + @unittest.skipUnless(HAS_SCAPY, 'scapy not installed') + def test_scapy_engine_finds_no_streams_for_this_capture(self) -> None: + # Scapy does not recognise this capture's link-layer type and hands every + # frame back as one opaque ``Raw`` layer with no TCP inside, so it correctly + # finds no TCP stream to follow. Zero is therefore a capability gap, not the + # GH-399 bug -- and the second assertion pins the *cause*, so a future scapy + # that learns this link type fails here loudly instead of silently drifting. + import pcapkit + + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + streams = self._follow(engine='scapy') + extractor = pcapkit.extract(fin=sample_path('in.pcap'), engine='scapy', + store=True, nofile=True) + + self.assertEqual(len(streams), 0) + self.assertFalse( + any(frame.haslayer('TCP') for frame in extractor.frame), + 'scapy dissected a TCP layer from in.pcap; the empty-stream assertion ' + 'above is no longer a capability gap and this test needs revisiting', + ) + + def test_pyshark_and_pypcap_fall_back_to_the_default_engine(self) -> None: + # Neither engine can trace TCP flows (PyShark has no reassembly adapter, + # PyPCAP does no dissection), so follow_tcp_stream redirects them to the + # default engine with an EngineWarning rather than raising. No PyShark or + # PyPCAP install is needed: the redirect happens before extraction begins. + from pcapkit.utilities.warnings import EngineWarning + + for engine in ('pyshark', 'pypcap'): + with self.subTest(engine=engine): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + streams = self._follow(engine=engine) + self.assertTrue(any(issubclass(w.category, EngineWarning) for w in caught)) + self.assertEqual(len(streams), IN_PCAP_TCP_STREAMS) + + @unittest.skipUnless(HAS_DPKT, 'dpkt not installed') + def test_dpkt_explicit_pcap_trace_format_is_downgraded_with_a_warning(self) -> None: + # The PCAP trace dumper cannot serialise DPKT's dict frames, so an explicit + # 'pcap' trace format is replaced with a dict-capable one, with a + # FormatWarning -- and the stream is still followed rather than crashing the + # extraction the way it did before GH-399 was fixed. + from pcapkit.utilities.warnings import FormatWarning + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + streams = self._follow(engine='dpkt', format='pcap') + + self.assertTrue(any(issubclass(w.category, FormatWarning) for w in caught)) + self.assertEqual(len(streams), IN_PCAP_TCP_STREAMS) + + @unittest.skipUnless(HAS_DPKT, 'dpkt not installed') + def test_dpkt_unset_trace_format_is_upgraded_quietly(self) -> None: + # An unset trace format is not a request, so upgrading it to a dict-capable + # format for DPKT does not warn -- only an explicit, unusable one does. + from pcapkit.utilities.warnings import FormatWarning + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + streams = self._follow(engine='dpkt') + + self.assertFalse(any(issubclass(w.category, FormatWarning) for w in caught)) + self.assertEqual(len(streams), IN_PCAP_TCP_STREAMS) + + def test_engine_without_a_reassembly_adapter_returns_no_streams(self) -> None: + # An engine pcapkit ships no reassembly adapter for -- a third-party one, or + # a built-in that grows dict frames -- must not be silently routed to the + # pcapkit adapter, which is precisely the GH-399 failure mode. It warns and + # returns nothing. Extractor is stubbed so the branch can be reached without + # registering a real engine. + from pcapkit.interface import misc + from pcapkit.utilities.warnings import EngineWarning + + class _FakeExtractor: + def __init__(self, **kwargs: object) -> None: + self.engine = types.SimpleNamespace(name='ThirdParty') + self.frame = () # type: tuple + self.trace = types.SimpleNamespace(tcp=[]) + + with mock.patch.object(misc, 'Extractor', _FakeExtractor): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + streams = misc.follow_tcp_stream(fin='ignored.pcap') + + self.assertEqual(streams, ()) + self.assertTrue(any(issubclass(w.category, EngineWarning) for w in caught)) if __name__ == '__main__': From e43067b62114d48e5e7a51fdf5db6cd7dc2f4c58 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 15 Sep 2026 19:50:31 -0400 Subject: [PATCH 2/3] tests: reference issues as #nnn, not GH-nnn Matches the majority form already used across the suite, and the one GitHub auto-links. --- pcapkit/interface/misc.py | 6 +++--- tests/interface/test_misc.py | 10 +++++----- tests/utilities/test_quiet_exceptions.py | 2 +- tests/utilities/test_warning_emission.py | 4 ++-- tests/utilities/test_warning_filters.py | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pcapkit/interface/misc.py b/pcapkit/interface/misc.py index a7f9bbb577..47fe60eb57 100644 --- a/pcapkit/interface/misc.py +++ b/pcapkit/interface/misc.py @@ -97,7 +97,7 @@ def follow_tcp_stream(fin: 'Optional[str]' = None, verbose: 'bool' = False, # NOTE: the DPKT and Scapy engines hand their frames to the flow tracer as plain # :obj:`dict`\\ s, which the PCAP trace dumper cannot re-serialise -- it reaches # for ``frame.packet`` and dies with ``AttributeError: 'dict' object has no - # attribute 'packet'`` (GH-399). The tracer defaults an unset ``format`` to + # attribute 'packet'`` (#399). The tracer defaults an unset ``format`` to # ``'pcap'``, so following a stream through either engine crashes *during # extraction*, before the reassembly below ever runs. :class:`Extractor # ` already substitutes a dict-capable @@ -123,7 +123,7 @@ def follow_tcp_stream(fin: 'Optional[str]' = None, verbose: 'bool' = False, # never its name -- so the historical ``extraction.engine == 'dpkt'`` compared an # object against a string and was *always* :data:`False`. Every capture then fell # through to the pcapkit adapter, which crashed on DPKT frames and silently - # returned no streams on Scapy frames (GH-399). Dispatch on the engine *type* + # returned no streams on Scapy frames (#399). Dispatch on the engine *type* # instead, and via :func:`isinstance` so that a third-party engine subclassing a # built-in still reaches the adapter that matches its frames. # @@ -152,7 +152,7 @@ def follow_tcp_stream(fin: 'Optional[str]' = None, verbose: 'bool' = False, tcp_reassembly = cast('ReassemblyAdapter', tk_pypcapfile.tcp_reassembly) else: # A third-party engine pcapkit ships no reassembly adapter for. Falling back - # to the pcapkit adapter is exactly the GH-399 failure mode -- a wrong or empty + # to the pcapkit adapter is exactly the #399 failure mode -- a wrong or empty # result indistinguishable from a real one -- so warn and return no streams # rather than reassemble frames whose shape we cannot parse. warn(f'unsupported extraction engine for TCP stream following: {exeng.name}; ' diff --git a/tests/interface/test_misc.py b/tests/interface/test_misc.py index ba19ae868c..7f940ed824 100644 --- a/tests/interface/test_misc.py +++ b/tests/interface/test_misc.py @@ -32,7 +32,7 @@ class FollowTCPStreamTests(unittest.TestCase): """:func:`~pcapkit.interface.misc.follow_tcp_stream` per extraction engine. ``examples/captures/in.pcap`` (committed, so unit-tier safe) holds three TCP - conversations. GH-399: the reassembly adapter was chosen by comparing the + conversations. #399: the reassembly adapter was chosen by comparing the engine *instance* to a string -- a comparison that is never true -- so every engine silently used the pcapkit adapter regardless of which engine ran. That crashed on DPKT frames (``AttributeError: 'dict' object has no attribute @@ -77,7 +77,7 @@ def test_default_engine_finds_every_tcp_stream(self) -> None: def test_dpkt_engine_matches_the_default_engine(self) -> None: # DPKT dissects in.pcap's Ethernet/IP/TCP in full, so it must agree with the # native engine on both the stream count and the reassembled bytes. This is - # the regression guard for GH-399: before the fix the DPKT frame reached the + # the regression guard for #399: before the fix the DPKT frame reached the # pcapkit adapter and raised AttributeError, and the counted-vs-uncounted # call convention is what makes the reassembled numbering line up. with warnings.catch_warnings(): @@ -96,7 +96,7 @@ def test_scapy_engine_finds_no_streams_for_this_capture(self) -> None: # Scapy does not recognise this capture's link-layer type and hands every # frame back as one opaque ``Raw`` layer with no TCP inside, so it correctly # finds no TCP stream to follow. Zero is therefore a capability gap, not the - # GH-399 bug -- and the second assertion pins the *cause*, so a future scapy + # #399 bug -- and the second assertion pins the *cause*, so a future scapy # that learns this link type fails here loudly instead of silently drifting. import pcapkit @@ -133,7 +133,7 @@ def test_dpkt_explicit_pcap_trace_format_is_downgraded_with_a_warning(self) -> N # The PCAP trace dumper cannot serialise DPKT's dict frames, so an explicit # 'pcap' trace format is replaced with a dict-capable one, with a # FormatWarning -- and the stream is still followed rather than crashing the - # extraction the way it did before GH-399 was fixed. + # extraction the way it did before #399 was fixed. from pcapkit.utilities.warnings import FormatWarning with warnings.catch_warnings(record=True) as caught: @@ -159,7 +159,7 @@ def test_dpkt_unset_trace_format_is_upgraded_quietly(self) -> None: def test_engine_without_a_reassembly_adapter_returns_no_streams(self) -> None: # An engine pcapkit ships no reassembly adapter for -- a third-party one, or # a built-in that grows dict frames -- must not be silently routed to the - # pcapkit adapter, which is precisely the GH-399 failure mode. It warns and + # pcapkit adapter, which is precisely the #399 failure mode. It warns and # returns nothing. Extractor is stubbed so the branch can be reached without # registering a real engine. from pcapkit.interface import misc diff --git a/tests/utilities/test_quiet_exceptions.py b/tests/utilities/test_quiet_exceptions.py index e72ae6d70e..e130b156ff 100644 --- a/tests/utilities/test_quiet_exceptions.py +++ b/tests/utilities/test_quiet_exceptions.py @@ -1,4 +1,4 @@ -"""Regression tests for GH-362 -- ``quiet=True`` must mean *emit nothing*. +"""Regression tests for #362 -- ``quiet=True`` must mean *emit nothing*. ``BaseError.__init__`` used to treat ``quiet`` as "log at ``ERROR`` instead of ``CRITICAL``", so every absent-key lookup through diff --git a/tests/utilities/test_warning_emission.py b/tests/utilities/test_warning_emission.py index 119fb33eab..1dc38c8b97 100644 --- a/tests/utilities/test_warning_emission.py +++ b/tests/utilities/test_warning_emission.py @@ -1,4 +1,4 @@ -"""Regression tests for GH-363 -- one :func:`~pcapkit.utilities.warnings.warn` +"""Regression tests for #363 -- one :func:`~pcapkit.utilities.warnings.warn` call must produce exactly one record on each channel. The emission model, which these tests pin: @@ -13,7 +13,7 @@ Before the fix the counts were: 1 outside development mode (the :mod:`warnings` emission was swallowed by the filter the constructor installed -- -see GH-364) and 3 in development mode (two log records, from ``warn()`` and again +see #364) and 3 in development mode (two log records, from ``warn()`` and again from ``BaseWarning.__init__``, plus the :mod:`warnings` emission). """ diff --git a/tests/utilities/test_warning_filters.py b/tests/utilities/test_warning_filters.py index efcd5fb510..c19e9e8cff 100644 --- a/tests/utilities/test_warning_filters.py +++ b/tests/utilities/test_warning_filters.py @@ -1,4 +1,4 @@ -"""Regression tests for GH-364 -- constructing a pcapkit warning must not touch +"""Regression tests for #364 -- constructing a pcapkit warning must not touch the process-global :data:`warnings.filters`. ``BaseWarning.__init__`` used to call ``warnings.simplefilter('ignore', From ea800298d0eb6ae3386af3f448270e21db874187 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 15 Sep 2026 20:14:18 -0400 Subject: [PATCH 3/3] types: make Formats name every format the registries accept Copilot's review on #402 was right, and the gap is pre-existing rather than introduced -- this change merely made it observable, since the dpkt/scapy trace workaround has to compare against 'cap'. `Formats` declared four values, while `Extractor.__output__` and `TraceFlowBase.__output__` both register eight: cap, json, pcap, plist, text, tree, txt, xml. So 'cap' and the 'txt'/'xml'/'text' aliases were accepted at runtime but unspellable for a type checker, and a caller passing one got a spurious error. Both declarations now match the registries exactly, verified against the live `__output__` keys rather than by eye. Two of those keys are defective for output -- 'pcap'/'cap' raise TypeError and 'text' raises AttributeError from dictdumper -- but they are registered, so they are part of the declared surface; the type should describe what the API accepts, not which paths happen to work. Both are pre-existing and unfiled. --- pcapkit/foundation/extraction.py | 8 +++++++- pcapkit/interface/misc.py | 9 ++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index a87728e4cc..11a19a7f29 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -60,7 +60,13 @@ from pcapkit.protocols.misc.pcapng import PCAPNG from pcapkit.protocols.protocol import ProtocolBase as Protocol - Formats = Literal['pcap', 'json', 'tree', 'plist'] + #: Every key registered in :attr:`Extractor.__output__` and in + #: :attr:`TraceFlowBase.__output__ + #: ` -- the two + #: registries expose the same eight keys. This used to name only four of them, + #: which made ``'cap'`` and the ``'txt'``/``'xml'`` aliases unspellable for a + #: type checker even though every one of them is accepted at runtime. + Formats = Literal['pcap', 'cap', 'json', 'tree', 'text', 'txt', 'plist', 'xml'] # NOTE: this alias is duplicated verbatim in ``pcapkit.interface.misc``; both # copies need updating when a new engine lands. The duplication predates the # engines added here and is left as-is on purpose. diff --git a/pcapkit/interface/misc.py b/pcapkit/interface/misc.py index 47fe60eb57..8f3281253f 100644 --- a/pcapkit/interface/misc.py +++ b/pcapkit/interface/misc.py @@ -37,7 +37,14 @@ ReassemblyAdapter = Callable[..., 'Optional[TCP_Data]'] ByteOrder = Literal['little', 'big'] - Formats = Literal['pcap', 'json', 'tree', 'plist'] + #: Every key registered in :attr:`Extractor.__output__ + #: ` and in + #: :attr:`TraceFlowBase.__output__ + #: ` -- the two + #: registries expose the same eight keys. This used to name only four of them, + #: which made ``'cap'`` and the ``'txt'``/``'xml'`` aliases unspellable for a + #: type checker even though every one of them is accepted at runtime. + Formats = Literal['pcap', 'cap', 'json', 'tree', 'text', 'txt', 'plist', 'xml'] # NOTE: this alias duplicates the one in ``pcapkit.foundation.extraction``; # both copies need updating when a new engine lands. Engines = Literal['default', 'pcapkit', 'dpkt', 'scapy', 'pyshark', 'pypcap', 'pypcapfile']