Skip to content

fix(foundation): restore record_header as the shared global-header step for third-party engines - #1149

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1104-record-header
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1104-record-header

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort). Ran these with the Makefile flags on the four source files: no new pylint codes, the same 2 pre-existing mypy errors in scapy.py, and isort clean.
  • make test passes, and a test case covers the change. Only the modules listed below were run.
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1104 (option 3). DPKT, Scapy and PyShark call ext.record_header() at setup again, as they did before 429f1a09a/74707ebc8. That call also sets _offmt, so the per-frame _offmt assignments were redundant and are removed.

Behaviour change (all three engines): JSON, plist and tree output gains one leading record, Global Header for PCAP or Section Header 1 for PCAP-NG. The record is identical to the default engine's. With files=True, a Global Header.<ext> file is written as well. The frame records are unchanged. Extractor.format now works on a capture with no frames, where it used to raise AttributeError. DPKT still takes the link type from reader.datalink(), because a PCAP-NG section header carries none.

Probe (in.pcap / dhcp.pcapng / a 0-frame PCAP, format='json'):

  • before: dpkt/scapy → ['Frame 1', …]; 0 frames → .format raises AttributeError: ... '_offmt'
  • after: ['Global Header', 'Frame 1', …] / ['Section Header 1', …]; 0 frames → .format == 'json'

Not #1127: format='pcap' fails on every engine, the default one included, at extraction.py:1193. Extractor.__init__ builds PCAPIO(ofnm) there, before any engine or header step runs. So this change cannot fix it.

Tests: new tests/foundation/test_extraction_record_header_unit.py gives 10 passed, and 10 failed with the source diff reverse-applied. PyShark is installed, but it is unsupported on 3.14, so it is driven through a stand-in module. test_runtime_engines.py was edited: its stand-in extractor gains record_header, and the DPKT _offmt pin now expects the header-time assignment. Modules run: runtime_engines 5 passed, test_extraction 22, offmt_unit 3, no_eof 11, scapy_engine 2, pyshark_engine 8+1 skipped, integration engine_parity 5, engine_runtime 4, output_formats 13+1 skipped, files_output_naming 4, cli_subprocess 14, tests/project 385 passed, 1 skipped.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on d3f88e74d: GOOD TO GO (ran on Sonnet; author Opus)

  • The ruling on chore(foundation): decide the fate of the unused public Extractor.record_header #1104 is in. DPKT and Scapy output now starts with Global Header (PCAP) or Section Header 1 (PCAP-NG), and that record matches the default engine's. Each output gains exactly one record, from 6 to 7 for in.pcap and from 4 to 5 for dhcp.pcapng. Every frame record hashes the same as on main, so the rewind causes no double read and no frame is renumbered.
  • Zero-frame captures: .format returns json on every engine. On main, DPKT and Scapy raise AttributeError: _offmt.
  • files=True: a header file is written next to the frame files.
  • Tests: the new module and test_runtime_engines fail without the fix. The edited DPKT test swaps a pin on the removed per-frame _offmt for a call check, which is legitimate.
  • Confirmed separate: format='pcap' (fix(scapy): the scapy engine fails with format='pcap' #1127) raises on every engine.

Nits:

  • The PR body's extraction.py:1193 is now line 1202.
  • No test pins PCAP-NG header equality or a PCAP-NG zero-frame capture.
  • PyShark is tested only through a stand-in, because it cannot run on Python 3.14.

@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 6, 2026
@JarryShaw
JarryShaw force-pushed the fix/1104-record-header branch from d3f88e7 to d99ebcc Compare October 6, 2026 21:48
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 6, 2026
@JarryShaw
JarryShaw force-pushed the fix/1104-record-header branch from d99ebcc to afdf756 Compare October 6, 2026 21:50
@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 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 86.80% (unit tier, Python 3.14, 1f3ac16e0, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1088 2342 899 89.73%
pcapkit/corekit 1873 382 578 65 76.46%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2422 104 842 40 94.24%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15659 444 3944 276 95.96%
pcapkit/toolkit 487 142 144 12 67.04%
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

Copy link
Copy Markdown
Owner Author

Cross-review verdict on afdf756fc: NEEDS CHANGES (ran on Fable; PR author Opus, round-2 fix author Sonnet)

  • Drop the new HAS_PYSHARK skip guard. With it, the two PySharkHeaderStepTests run on no CI leg at all. The test matrix installs .[test,DPKT,crypto,NGAP] (unit-tests.yml:118), so the guard skips them on all 5 legs. The PyShark engine cell runs a fixed TEST_PATHS (:610) that omits this file. The parity job installs pyshark (:761) but runs only fixture_tier_paths(), which does not include it either; I checked. The guard is also unneeded, because the test stubs pyshark in sys.modules and pcapkit/toolkit/pyshark.py imports it only under TYPE_CHECKING.
  • Keep the fix itself. import_module plus patch.object is confirmed correct. 3.10's mock._dot_lookup re-reads parent-module attributes, while 3.11+ resolves through pkgutil.resolve_name. A stale-parent state reproduces the exact CI AttributeError under the 3.10 resolver. The patch has teeth: misdirecting it makes the global-header test error out.
  • Correction to the round-2 report: tests/toolkit/test_pyshark_unit.py and tests/project/test_pyshark_encap_map.py contain no patch( calls. Of the siblings named, only tests/foundation/engines/test_runtime_engines.py (:444-477) carries the same latent 3.10 fragility, and it is HAS_RUNTIME-gated.

@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 6, 2026
…ep for third-party engines

The DPKT, Scapy and PyShark engines call Extractor.record_header() at
setup again, as they did before 429f1a0 and 74707eb. Their output
now opens with the header record the built-in engines write ("Global
Header" for PCAP, "Section Header 1" for PCAP-NG), and _offmt is set at
header time, so Extractor.format works on a capture with no frames.

The per-frame _offmt assignments in the three engines are dropped as
redundant. test_runtime_engines' stand-in extractor gains a
record_header mock, and its DPKT read_frame _offmt pin is updated.

Closes #1104
@JarryShaw
JarryShaw force-pushed the fix/1104-record-header branch from afdf756 to 1f3ac16 Compare October 6, 2026 22:15
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 1f3ac16e0: GOOD TO GO (ran on Haiku, round 3; Fable was intended, but hit a 429 rate limit before running any check; PR author Opus, fix author Sonnet)

  • Delta: afdf756fc..1f3ac16e0 removes exactly the HAS_PYSHARK constant and its skipUnless guard. Net change against d3f88e74d is +8/−1 in tests/foundation/test_extraction_record_header_unit.py only. (The reviewer reported this as "2 deletions"; I re-measured it.)
  • The round-2 finding is resolved: I ran the module with sys.modules['pyshark'] = None and confirmed import pyshark is halted. PySharkHeaderStepTests ran 2 with 0 failures and 0 skips. The reviewer found no skip or deselect for this file in tests/conftest.py, pyproject.toml or tests/_tiers.py. tests/_dependency_gates.py does not list the file.
  • tests/test_tier_guard.py: 109 passed.
  • CI on 1f3ac16e0: fail=0, inc=0, and Python 3.10 is SUCCESS, which is the leg that failed on d3f88e74d.
  • Follow-up, out of scope: tests/foundation/engines/test_runtime_engines.py (:444-477) uses the same dotted-string patch form. It is latent there because no CI job reaches that file.

@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 6, 2026
@JarryShaw
JarryShaw merged commit 7e11f50 into main Oct 6, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1104-record-header branch October 6, 2026 22:38
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 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.

chore(foundation): decide the fate of the unused public Extractor.record_header

1 participant