Skip to content

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

Description

@JarryShaw

Is your feature request related to a problem? Please describe.
Extractor.record_header (pcapkit/foundation/extraction.py:840) is a public method, but nothing in pcapkit/ calls it. Only tests/foundation/test_extraction.py:847-861 and tests/foundation/test_extraction_no_eof.py:454 use it. #1022 changed only its typing.

Describe the solution you'd like
This needs the maintainer's decision. The options are:

  1. Keep it and document it as public API.
  2. Deprecate it for one release, then remove it.
  3. Remove it now. This would be a breaking change.

Additional context
Found while verifying the #719 follow-up defects; see the #1090–#1102 wave.

Activity

  1. added
    choreMaintenance work: tooling, repo hygiene, no library behaviour change
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    on Oct 6, 2026
  2. JarryShaw commented on Oct 6, 2026

    @JarryShaw
    OwnerAuthor

    The PCAP read path has never gone through record_header. The PCAP engine reads its own global header inside run() (engines/pcap.py:113); the PCAP-NG engine does the same. Git history shows what changed:

    • On 2023-04-14, 232a5e6a7 and 2afffde74 made the DPKT, Scapy and PyShark engines call ext.record_header(). That borrowed the built-in engine to parse and dump the global header.
    • On 2023-04-27, 429f1a09a ("no need to call builtin engine") removed those calls from DPKT and PyShark, and 74707ebc8 removed the one in Scapy. DPKT now sets its own _offmt (engines/dpkt.py:179).
    • Since then, nothing has called record_header, and the Scapy engine was left without the _offmt assignment that the header dump used to set. That is fix(foundation): Extractor.output message, make_name registry pollution, scapy engine _offmt #1095's third defect.

    So the options are:

    1. Remove record_header (breaking).
    2. Keep it as public API.
    3. Restore it as the shared header step, so the third-party engines also dump the global header. This is a behaviour change: their output would gain a Global Header record.

    Which would you prefer?

  3. JarryShaw commented on Oct 6, 2026

    @JarryShaw
    OwnerAuthor

    What's your recommendation? I think for a certain period of time, I used to do option 3.

  4. JarryShaw commented on Oct 6, 2026

    @JarryShaw
    OwnerAuthor

    I'd go with option 3: restore record_header as the shared header step for the third-party engines.

    • Consistent output. None of the DPKT, Scapy or PyShark engines reads or dumps the global header today; each has 0 references to Global Header, record_header or _gbhdr. A JSON, plist or tree dump therefore has a Global Header record with the built-in engines and none with a third-party one.
    • Fixes the follow-on gaps.
    • It is the historical design. record_header dates from 2023-04-14 (232a5e6a7), and its only callers were removed on 2023-04-27.

    The cost is a behaviour change: third-party dumps gain a Global Header record. It would go in the changelog. If you agree, I'll do it after #1113 merges (same files) and fold #1127 into it.

  5. JarryShaw commented on Oct 6, 2026

    @JarryShaw
    OwnerAuthor

    cool, take (3) then.

  6. JarryShaw commented on Oct 6, 2026

    @JarryShaw
    OwnerAuthor

    Ruling from the maintainer: option (3). record_header becomes the shared global-header step for the DPKT, Scapy and PyShark engines again, so their dumps gain the Global Header record. The changelog will note this behaviour change.

    Blocked until #1113 merges. It edits engines/scapy.py and extraction.py. #1127, Scapy with format='pcap', will be checked against this fix and folded in if it shares the cause.

  7. added
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Oct 6, 2026
  8. added 3 commits that reference this issue on Oct 6, 2026
    d99ebcc
    afdf756
    1f3ac16
  9. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    choreMaintenance work: tooling, repo hygiene, no library behaviour change

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions