Skip to content

fix(dumpkit)!: output writers drop or corrupt what the packets carried - #1305

Merged
JarryShaw merged 1 commit into
mainfrom
fix/dumper-roundtrip-wave2
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/dumper-roundtrip-wave2

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

  • Searched for similar pull requests
  • Followed the coding style: ran pylint (Makefile flags), mypy and isort on the 3 source files. No new findings; the bad-super-call at common.py:437 was there before.
  • make test passes, and a test case covers the change. I ran only the modules listed below, not the full suite.
  • Changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1255
Closes #1257
Closes #1258
Closes #1261
Closes #1262
Closes #1263

BREAKING (dump schema):

  • An OrderedMultiDict, which covers every option list, now dumps as [{"NOP": …}, {"MSS": …}, {"NOP": …}] in wire order. It used to dump as {"NOP": [..], "MSS": [..]}. MultiDict keeps no cross-key order, so it stays grouped.
  • In plist, None dumps as dictdumper's own {type: NoneType, value: None}.
  • In plist, a string with an XML-illegal character becomes <data> (UTF-8).
  • A timezone dumps as its offset in seconds, like timedelta.
  • NULL and NO_VALUE dump as '<NULL>' and '<NO_VALUE>'.

Probe (/tmp/rtissues/dumpkit-repro.py): on 11ae68ebf, D1, D3, D4, D7, D8 and D9 all report REPRO. After the fix all six report NOREPRO. The D8 regex expects <string>, so I checked it with plistlib.loads. All 16 examples/captures/*.pcap still write byte-identical PCAP. Every *.pcap* capture's plist and json output now parses.

Tests:

  • The new module is tests/dumpkit/test_dumper_roundtrip_unit.py. It passes 7 tests with the fix. Without the fix, 7 tests fail; only the v2.4 subtest passes.
  • I edited four existing modules where they pinned the grouped OMD output, and the fallback stub's str path.
  • Every tests/dumpkit module passes. So do 30 dump-related modules under foundation, integration, interface, protocols and toolkit.
  • tests/project: 389 passed, 1 skipped.
  • run_unittest_leg.py dumpkit: 216 tests, 0 failures.

Comment thread pcapkit/dumpkit/common.py Fixed
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on e63ae1357: GOOD TO GO (Sonnet reviewed; the author ran on Opus).

  • The checks pass. D1, D3, D4, D7 and D9 no longer reproduce. Of these:
    • D1: the version is now preserved.
    • D7: astral characters are written as surrogate pairs.
    • D9: entries come out in wire order.
    • D8, checked by hand: XML-illegal strings are written as <data> and parse.
  • Fixtures. PCAP output is byte-identical for all 17 PCAP fixtures. json.load succeeds on all 23. plist parses once the existing fractional-second dates (DictDumper#122) are trimmed.
  • Grouped form. Apart from the four edited tests, nothing consumes the old grouped OrderedMultiDict form. The dumpkit leg runs 216 tests with 0 failures, and all 18 dump-producing modules pass.
  • Nits, all pre-existing:
    • Illegal XML characters in a plist key still produce malformed XML.
    • \r comes back as \n in plist.
    • A real string '<NULL>' dumps the same as the sentinel.
  • Before merge: CodeQL flags _XML_ILLEGAL (common.py:53) because the pattern is a plain string literal, so the control-character ranges reach re as raw characters. Switching to a raw string with the same ranges should clear it. That fix is being made now.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
- pcap: forward the input's global-header version to PCAPIO (#1255).
- plist: write None values and list items instead of skipping them (#1257).
- Networks, NULL, NO_VALUE and timezones get an explicit rendering, and
  _append_fallback reads the slots of every base class (#1258).
- json: write characters above U+FFFF as surrogate pairs (#1261).
- plist: write a string XML 1.0 cannot spell as <data> (#1262).
- OrderedMultiDict dumps as a list of single-key objects in insertion
  order (#1263). BREAKING: changes the dump schema of every option list.

Closes #1255
Closes #1257
Closes #1258
Closes #1261
Closes #1262
Closes #1263
@JarryShaw
JarryShaw force-pushed the fix/dumper-roundtrip-wave2 branch from e63ae13 to 0798570 Compare October 7, 2026 18:41
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict on 0798570f9: GOOD TO GO. This push differs from e63ae1357 (cross-reviewed GOOD TO GO) by one line, common.py:53: _XML_ILLEGAL is now a raw string with escaped ranges, so CodeQL no longer sees literal control characters in the pattern. I compared old and new over all 1,114,112 code points and they match on every one. Python 3.10 to 3.14 accept the surrogate range, and the dumpkit leg passes (216 tests, 0 failures).

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.92% (unit tier, Python 3.14, 0798570f9, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1034 2342 845 90.24%
pcapkit/corekit 1878 91 580 22 94.34%
pcapkit/dumpkit 189 1 68 1 99.22%
pcapkit/foundation 2442 102 852 39 94.38%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15777 190 4036 165 98.18%
pcapkit/toolkit 487 65 144 1 85.42%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 9093061 into main Oct 7, 2026
36 checks passed
@JarryShaw
JarryShaw deleted the fix/dumper-roundtrip-wave2 branch October 7, 2026 20:04
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
JarryShaw added a commit that referenced this pull request Oct 7, 2026
…d fills the disk

- _append_fallback writes a class, module or routine as its qualified
  name instead of expanding it through vars(), which since #1305 turned a
  class's mappingproxy into a dict and walked the whole type graph.
- An object already on the current expansion path is written as a
  <circular reference: ...> marker, so a cycle terminates; a shared
  object is still expanded in full each time it is met.
- A top-level fallback object expanding into more than FALLBACK_LIMIT
  (10000) objects raises UnsupportedCall instead of writing on.
- test_runtime_engines: expect the version= keyword #1305 added to
  PCAP's _open_output call.

Closes #1372
JarryShaw added a commit that referenced this pull request Oct 8, 2026
…d fills the disk (#1381)

- _append_fallback writes a class, module or routine as its qualified
  name instead of expanding it through vars(), which since #1305 turned a
  class's mappingproxy into a dict and walked the whole type graph.
- An object already on the current expansion path is written as a
  <circular reference: ...> marker, so a cycle terminates; a shared
  object is still expanded in full each time it is met.
- A top-level fallback object expanding into more than FALLBACK_LIMIT
  (10000) objects raises UnsupportedCall instead of writing on.
- test_runtime_engines: expect the version= keyword #1305 added to
  PCAP's _open_output call.

Closes #1372
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 25, 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) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

2 participants