Skip to content

fix(engines): the pyshark engine skips non-packet PCAP-NG blocks (#1515) - #1531

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1515-pyshark-journal-block
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1515-pyshark-journal-block

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

tshark numbers every record it reads. A PCAP-NG systemd Journal Export Block, Custom Block or Sysdig Event Block is a frame to tshark 4.6.9, but not to the default engine. read_frame now skips a record unless its frame layer carries frame.encap_type. Wireshark adds that field for packet records only, in 4.2.2 and 4.6.9 (epan/dissectors/packet-frame.c). The layer is found by name, because pyshark's frame_info is pkt_comment on a packet that carries a comment. The verbose handler read frame_info.protocols, so extract(fin='test.pcapng', engine='pyshark', verbose=True) raised AttributeError on frame 1. It now reads the frame layer too. Frames are numbered by count, not by tshark's number.

  • The Gap(1515) rows are deleted. test.pcapng/pyshark/* passes on Python 3.11 + tshark 4.6.9; without the fix it fails with 6 frames != 5.
  • New tests/foundation/engines/test_pyshark_non_packet_unit.py: all 4 tests fail without the fix.
  • test_runtime_engines.py and test_extraction_record_header_unit.py: their fake packets now carry frame_info and layers, which pyshark 0.6's Packet always sets. test_runtime_engines.py expects frame 1 rather than tshark's 7.
  • Local runs: tests/foundation, tests/toolkit, tests/integration, tests/project, every tests/test_*.py.

Closes #1515

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1515-pyshark-journal-block branch from c191631 to e46b765 Compare October 9, 2026 22:57
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 9, 2026
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at e46b765b2. Cross-reviewed on Sonnet; the author ran Opus.

  • frame.encap_type is added only to REC_TYPE_PACKET records, verified in Wireshark's epan/dissectors/packet-frame.c:
    • wireshark-4.2.2: guard at :1001, field added at :1002.
    • v4.6.9: guard at :828, field added at :890.
    • The same guard sits at 3.6.0, 4.0.0, 4.4.0 and 4.4.9.
    • Crafted captures behave as expected. An unknown link type still carries the field (value 0), as do per-packet encaps, SPBs and obsolete PBs. Journal, custom and sysdig records do not.
  • Frame counts and timestamps now agree with the default engine on all 23 sample captures: test.pcapng is 5 vs 5, against 5 vs 6 on main. They also agree on 6 crafted captures.
  • Verbose mode prints frames 1–5 on test.pcapng; main raises AttributeError.
  • Tests: fail-before is 9. Four mutants are each killed by the new tests. The updated test_runtime_engines.py test is stricter than before, not weaker.

Not covered by this PR, and passed to #1527: toolkit/pyshark.py :331 (packet2dict), :398 and :422 still read frame_info, which on a commented packet is the comment. :413 indexes flows by tshark's frame number.

@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 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at e46b765b2. CI is red, which supersedes the earlier GOOD TO GO.

The Python 3.12 job (114063798474) fails one test, tests/foundation/test_extraction_record_header_unit.py::PySharkHeaderStepTests::test_pcap_output_opens_with_the_global_header, with AttributeError: 'types.SimpleNamespace' object has no attribute 'frame_info'. The error comes from engines/pyshark.py:73 (_is_packet → _frame_layer, for layer in (packet.frame_info, *packet.layers)).

That test's fake packet has no frame_info attribute. The new lookup now requires one, and the worker's selection covered tests/foundation/engines but not the rest of tests/foundation. The fix goes back to the worker.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1515-pyshark-journal-block branch from e46b765 to 1ec6f74 Compare October 9, 2026 23:37
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 1ec6f74ec. The CI failure is fixed.

The delta from e46b765b2 is test-only: tests/foundation/test_extraction_record_header_unit.py, +3/−1. The library is unchanged (0 lines of pcapkit/ diff).

  • That test's fake packet now carries a frame_info frame layer.
  • Real pyshark 0.6 packets always have one: Packet.__init__ sets self.frame_info (packet/packet.py:33).
  • I re-ran that file plus test_pyshark_non_packet_unit.py and test_runtime_engines.py on this head: 19 passed.
  • The worker's runs of all of tests/foundation, tests/toolkit, tests/integration, tests/project and the top-level modules are green.

The Sonnet review of e46b765b2 still stands for the engine change.

Merge note: this and #1527 conflict only on their adjacent Gap(1514) and Gap(1515) row deletions in the agreement module. Whichever lands second drops both rows.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 90.02% (unit tier, Python 3.14, 28c9d96dc, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 842 2356 756 92.09%
pcapkit/corekit 2204 67 712 34 96.26%
pcapkit/dumpkit 258 2 90 2 98.85%
pcapkit/foundation 2809 131 992 48 94.24%
pcapkit/interface 108 7 36 5 91.67%
pcapkit/protocols 16992 229 4546 190 98.03%
pcapkit/toolkit 635 98 198 5 83.31%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Resolve conflicts

@JarryShaw

Copy link
Copy Markdown
Owner Author

On it. The worker is rebasing onto main (c3d412015). It conflicts with #1527 in engines/pyshark.py and with the Gap table in tests/foundation/engines/test_engine_agreement_runtime.py. Both fixes are being kept, and the rebased head gets a delta review.

@JarryShaw
JarryShaw force-pushed the fix/1515-pyshark-journal-block branch from 1ec6f74 to bec58be Compare October 10, 2026 02:04
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at bec58be12. This was a Sonnet delta review; the author ran Opus. The rebase and the engine code are clean. One test needs tightening.

  • Rebase is clean: the range-diff only changes the context around the removed Gap(1515) row, and the added and removed lines match at -U0 (5 files, +207/−16). fix(toolkit): accept tshark's True/False TCP flags in the pyshark trace (#1514) #1527's count=ext._frnum is intact at engines/pyshark.py:314, and no Gap(1514) or Gap(1515) remains.
  • Behaviour is confirmed: on tcp.pcap with a Journal Export Block inserted at each position, pyshark and the default engine both trace [1, 2]. With main's read_frame grafted in, pyshark traces [2, 3] or [1, 3]. In the 3.11 venv the pyshark tests give 51 passed and the agreement module 3 passed.

To fix: tests/toolkit/test_pyshark_unit.py:654 is assertIn(traced, ([2, 3], [1, 2])), a hedge from #1527 for before this PR landed. Its docstring (:620) says so. Now that #1527 is on main, pin self.assertEqual(traced, [1, 2]) and drop the "Without #1531" sentence. That makes it a real fail-before test.

Non-blocking: _frame_layer is duplicated at toolkit/pyshark.py:400 and engines/pyshark.py:38. It can be a follow-up.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
tshark numbers every record it reads. A systemd Journal Export Block,
Custom Block or Sysdig Event Block is a frame to it, but not to the
default engine, so test.pcapng read as 6 frames instead of 5. Every
frame after the journal block was also off by one.

- read_frame skips a record unless its frame layer carries
  frame.encap_type. Wireshark adds that field for packet records only,
  in 4.2.2 and 4.6.9.
- The frame layer is looked up by name, because frame_info is
  pkt_comment on a packet that carries a comment. The verbose handler
  read protocols from frame_info and raised AttributeError on such a
  packet; it now reads the frame layer too.
- Frames are numbered by count, as the default engine numbers them,
  not by tshark's frame number.
- Drop the Gap(1515) rows from the engine agreement module, and pin
  the engine's traced frame count to [1, 2] in the toolkit tests.

Closes #1515
@JarryShaw
JarryShaw force-pushed the fix/1515-pyshark-journal-block branch from bec58be to 28c9d96 Compare October 10, 2026 02:17
@JarryShaw JarryShaw removed the review: needs-changes Cross-review at the current head says changes are required; see the verdict comment label Oct 10, 2026
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 28c9d96dc. This was a Sonnet delta review; the author ran Opus.

  • Scope: the delta from bec58be12 is the requested test pin and nothing else (+3/−4). The range-diff against 16fbea054 shows only that hunk.
  • Fail-before: assertEqual(traced, [1, 2]) passes at this head. With main's engines/pyshark.py swapped in, it fails with [2, 3] != [1, 2] in both the 3.11 and the 3.14 venv. tests/toolkit/test_pyshark_unit.py gives 32 passed in each.
  • Earlier rounds: everything confirmed in rounds 1 and 2 still holds.

@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 10, 2026
@JarryShaw
JarryShaw merged commit ef83f83 into main Oct 10, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 10, 2026
@JarryShaw
JarryShaw deleted the fix/1515-pyshark-journal-block branch October 10, 2026 03:34
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(engines): pyshark engine counts a systemd Journal Export Block as a frame

1 participant