Fix TCP reassembly coordinates (#349); add an end-to-end test tier - #376
Merged
Merged
Conversation
Hole descriptors carried absolute sequence numbers from the toolkits, the first hole was seeded from a payload length, and submit() sliced a buffer indexed from its own initial sequence number with those bounds. On any capture with a SYN and a realistic ISN every slice landed past the end of the buffer, so an incomplete datagram was dropped silently instead of being returned with completed=False. Descriptors are now absolute and inclusive throughout, submit() is the only place they are converted into buffer offsets, and holes outside a buffer are dropped rather than indexing from its far end. A SYN no longer contributes its own sequence number to the payload, which had been prepending a NUL octet to complete datagrams. Closes #349.
The integration tier was three modules; the real end-to-end spectrums lived only as the assertion-free demonstrations in examples/legacy_smoke. Added seven modules covering report formats on disk, manual frame iteration, TCP and IP reassembly, flow tracing, engine parity, PCAP-NG including both byte orders, and the CLI as a subprocess. Assertions are on content rather than liveness: reassembled bodies match their own Content-Length, and the 331 traced flows partition all 1117 frames of http.pcap exactly once. Four tests are skipped against defects they would otherwise pin, each naming the file and line to fix.
test_sample_capture_reassembles_every_stream_byte_exactly reads test.pcap, which examples/generators/make_samples.py builds rather than the repository carrying it, so the unit-test workflow -- which deliberately runs without generated fixtures -- failed on every Python version with FileNotFoundError. Moved to tests/foundation/reassembly/test_tcp_runtime.py, matching the convention the ignore globs already encode: fixture-dependent cases live in *_runtime.py and *_regression.py, and the integration job generates the fixtures before running them. CI selection: 318 passed, 103 subtests passed, with no fixtures present.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core TCP reassembly logic and introduces a large new end-to-end test tier, warranting final human review despite the strong accompanying tests.
Pull request overview
This pull request fixes TCP reassembly coordinate handling so incomplete datagrams are no longer silently dropped (and off-by-one payload bounds are corrected), and it adds a substantial end-to-end integration test tier to validate real extractor behavior (reports, reassembly, tracing, CLI, and engine parity).
Changes:
- Correct TCP reassembly descriptor semantics (absolute sequence numbers, inclusive
last) across all toolkits and the TCP reassembler, including SYN sequence-number consumption and strict-mode fragment extraction. - Add a new
tests/integration/end-to-end suite (plus shared helpers) covering report formats, reassembly, traceflow, engine parity, PCAP-NG behavior, and CLI subprocess runs. - Expand/adjust unit tests to pin the corrected TCP descriptor contract and to add a runtime-gated reassembly test that relies on generated fixtures.
File summaries
| File | Description |
|---|---|
pcapkit/foundation/reassembly/tcp.py |
Fixes TCP reassembly coordinate conversion, hole-list handling, and strict-mode incomplete datagram extraction. |
pcapkit/foundation/reassembly/data/tcp.py |
Clarifies and documents Packet/HoleDescriptor bounds as absolute and inclusive. |
pcapkit/toolkit/pcap.py |
Makes last inclusive (seq + len - 1) in TCP reassembly descriptors. |
pcapkit/toolkit/pcapng.py |
Makes last inclusive (seq + len - 1) in TCP reassembly descriptors. |
pcapkit/toolkit/dpkt.py |
Makes last inclusive (seq + len - 1) in TCP reassembly descriptors. |
pcapkit/toolkit/scapy.py |
Makes last inclusive (seq + len - 1) in TCP reassembly descriptors. |
tests/foundation/reassembly/test_tcp.py |
Adds coordinate-system regression coverage and updates expectations around SYN sequence consumption. |
tests/foundation/reassembly/test_tcp_runtime.py |
Adds a runtime-gated end-to-end TCP reassembly test against a generated capture. |
tests/toolkit/test_pcap_unit.py |
Adds descriptor-contract tests and a seam test driving the reassembler with toolkit-built descriptors. |
tests/toolkit/test_scapy_unit.py |
Adds assertions pinning first/last semantics for scapy toolkit descriptors. |
tests/toolkit/test_dpkt_unit.py |
Updates assertions to pin inclusive last semantics for dpkt toolkit descriptors. |
tests/integration/_helpers.py |
Introduces shared integration scaffolding (dependency gates, temp dirs, report readers). |
tests/integration/test_output_formats.py |
Adds end-to-end assertions for tree/json/plist outputs and files=True outputs. |
tests/integration/test_frame_iteration.py |
Adds end-to-end manual iteration coverage and documents the currently-broken extraction limits behind a skip. |
tests/integration/test_reassembly_end_to_end.py |
Adds end-to-end TCP/IP reassembly assertions, with known-defect payload assertions behind a skip. |
tests/integration/test_traceflow_end_to_end.py |
Adds end-to-end flow tracing assertions, including a scale/partitioning check on http.pcap. |
tests/integration/test_engine_parity.py |
Adds parity checks across available engines (default/dpkt/scapy) for counts and selected behaviors. |
tests/integration/test_pcapng_end_to_end.py |
Adds end-to-end PCAP-NG fixture assertions and report round-trip checks (with known-defect skips). |
tests/integration/test_cli_subprocess.py |
Adds subprocess-based CLI end-to-end coverage for python -m pcapkit and (optionally) pcapkit-cli. |
examples/generators/legacy.py |
Updates generator documentation to reflect the corrected reassembly behavior and test strategy. |
Review details
- Files reviewed: 20/20 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.
JarryShaw
added a commit
that referenced
this pull request
Sep 15, 2026
The new verbose and debug-trail tests read arp.pcap, which the generators build rather than the repository carrying, so the unit-test workflow - which deliberately runs without generated fixtures - failed on every Python version with FileNotFoundError. Same trap as the reassembly case in #376. Switched to in.pcap, one of the six committed captures, and corrected the expected frame count from 2 to 6. Both tests belong in the unit tier: they assert logging behaviour, not capture contents, so any real capture will do. Verified by running the CI selection with only the committed captures present: 417 passed, 219 subtests passed.
JarryShaw
added a commit
that referenced
this pull request
Sep 15, 2026
) * logging: make the logger a library citizen, and use it pcapkit configured logging at import: a single flat logger named 'pcapkit', with a StreamHandler on stderr attached and the level forced from PCAPKIT_DEVMODE. Importing the library therefore hijacked the consumer's logging, and undoing it meant reaching into logger.handlers. There was also no way to change verbosity at runtime, since the environment variable is read once. Now a NullHandler and no level at import, so verbosity is inherited from the application, with the old stderr handler kept for PCAPKIT_DEVMODE as the opt-in path. Alongside the existing logger object: get_logger() for per-module children, configure() to set level, handler, stream, format or propagation at runtime and per logger name, reset() to return to library-neutral, and ensure_output() for the verbose= path. Seventeen modules take getLogger(__name__), so a consumer can silence pcapkit.foundation.registry while keeping pcapkit.foundation.extraction. The library was also nearly silent where it mattered and noisy where it did not. 38 info calls become debug - 34 of them registry bookkeeping, 4 extractor configuration - leaving exactly one info in the package, the pcapkit-vendor CLI progress line. Four print calls that were clearly meant to be logging, each self-documenting with a logging-fstring-interpolation suppression, become logger.debug with lazy % args. And 31 new debug calls cover what was undiagnosable: extractor lifecycle, engine selection and fallback, reassembly and trace-flow entry points. Nothing was added to per-frame or per-field loops. Behaviour changes a consumer could notice, all documented in a compatibility note on the new docs page: no stderr handler at import (restore with configure(logging.INFO, stream=sys.stderr)); registry messages invisible even at INFO; handler is no longer logger.handlers[0]; and verbose=True logs at debug, so ensure_output() attaches a stderr handler only when nothing up the chain would receive the record - otherwise Extractor(verbose=True) would have gone silent. warnings.py keeps its behaviour: the double-emit and the global simplefilter mutation are filed as #363 and #364 and want their own change. * logging: keep verbose frame output on stdout, not on the logger Converting the four verbose= handlers to logger.debug moved user-facing output onto the logger, which changed both its stream and its visibility: the CLI's -v frame chains went from stdout to stderr and only appeared if the consumer had configured a handler. tests/integration/test_cli_subprocess.py caught it. Those chains are a feature of the tool, not diagnostics, so they stay on print. That also removes the need for ensure_output() on the library path -- it existed only to stop verbose=True falling silent once the import-time handler was gone. The helper itself stays public and tested, since it is a reasonable thing for a consumer to call. Every genuine debug call added by this branch is untouched; only the four verbose handlers revert. Docs note and test updated to state the rule: verbose= is stdout, logging is diagnostics. Also converted register_sctp's logger.info to debug, which the branch had missed because SCTP landed on main after it was written -- the branch's own "no info on the registry path" test now passes. Full suite: 529 passed, 4 skipped, 303 subtests passed. * tests: keep the logging tests on a committed capture The new verbose and debug-trail tests read arp.pcap, which the generators build rather than the repository carrying, so the unit-test workflow - which deliberately runs without generated fixtures - failed on every Python version with FileNotFoundError. Same trap as the reassembly case in #376. Switched to in.pcap, one of the six committed captures, and corrected the expected frame count from 2 to 6. Both tests belong in the unit tier: they assert logging behaviour, not capture contents, so any real capture will do. Verified by running the CI selection with only the committed captures present: 417 passed, 219 subtests passed. * logging: do not disturb configuration the application already made Import called reset(), which detached whatever handlers the host application had attached to the pcapkit logger and forced NOTSET and propagate back on. That contradicts the point of the change: a library should inherit the application's configuration, not overwrite it because it was imported second. Import now only guarantees the logger has a handler, and the devmode branch checks membership before attaching, so re-execution still cannot stack handlers - which is what reset() was there for. Verified: an application that configures a handler, INFO and propagate=False before importing pcapkit keeps all three, and two reloads leave one handler. Also from review: - dropped `import logging` from four modules where reverting the verbose handlers to print left it unused (AST-verified, not just grepped: the name survives only in docstrings). - extraction and traceflow logged dumper.__name__, which make_dumper() always names 'DictDumper' whatever the format. They log the wrapped output class now. dictdumper exposes `kind` rather than `name`, but it is an instance property and both sites log before instantiation - and its value duplicates the `fmt` already on the same line, so the class name is what adds anything. - ensure_output's docstring no longer cites verbose= as its motivation, since that path deliberately stays on print, and notes nothing in pcapkit calls it. Full suite 529 passed, 4 skipped, 303 subtests; CI selection without generated captures 417 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #372, which GitHub closed automatically when its base branch
fix/pcapng-parser-defectswas deleted on merging #371. Same work, rebased ontomain, now with only its own three commits.TCP reassembly (#349)
Hole descriptors carried absolute sequence numbers from the toolkits, the first hole was seeded from a payload length, and
submit()sliced a buffer indexed from its own initial sequence number using those bounds. On any capture with a SYN and a realistic ISN every slice landed past the end of the buffer, so an incomplete datagram was dropped silently instead of coming back withcompleted=False— thecompleted=False/packet is Nonebranches of the public API were unreachable in practice.Measured, 5 frames with two segments permanently lost:
0xC0DE1234completed=False, fragments[10, 10, 10][10][10, 10, 10][11, 11, 10][10, 10, 10]Descriptors are absolute and inclusive throughout now, with
submit()the only place they become buffer offsets; holes outside a buffer are dropped rather than indexing from its far end. Absolute was chosen over relative becausehdllives onBuffer, shared acrossFragments, whereas a relative origin would have to live onFragment— whoseisnis revised downwards whenever a segment arrives below the data already buffered.A fourth symptom the issue did not name is also fixed: a complete stream whose SYN shares an acknowledgement number came back one octet too long, the SYN's own sequence number appearing as a leading NUL.
Four of the five toolkits needed the inclusive-bound change (
pysharkhas no reassembly at all), and four existing tests were pinning the broken behaviour.End-to-end tier
tests/integration/was three modules; the real end-to-end spectrums existed only as the assertion-free demonstrations inexamples/legacy_smoke/. Seven modules added — 60 tests, 73 subtests, 23s — covering report formats written to disk and read back (tree/json/plist,files=True), manual frame iteration, TCP and IP reassembly, flow tracing, engine parity acrossdefault/dpkt/scapy, PCAP-NG including both byte orders, and the CLI as a subprocess.Assertions are on content rather than liveness: reassembled bodies match their own
Content-Length, and the 331 traced flows ofhttp.pcappartition all 1117 frames exactly once.Four tests are skipped rather than pinning defects — the plist
<date>format,layer=/protocol=being ignored (#356), the IPv6 fragment offset (#352), and unescaped JSON mapping keys. Each names the file and line, so whoever fixes it can simply un-skip. That choice was deliberate:test_ipv6_extension_runtime.pyhad pinned a bug, which is why fixing #348 required editing a test.The unit tier stays fixture-free
The third commit moves the one end-to-end reassembly case into
tests/foundation/reassembly/test_tcp_runtime.py. It readstest.pcap, which the generators build rather than the repository carrying, and the unit workflow deliberately runs without generated fixtures — so on the first push it failed on all six Python versions withFileNotFoundError.*_runtime.pyis the convention the ignore globs already encode.Verification
Full suite on this branch, rebased onto current
main: 439 passed, 4 skipped, 191 subtests passed.Closes #349.