Skip to content

fix(pcapng): resolve byte order for a raw bytes option without self._byte set (#2353) - #2369

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2353-pcapng-byte-order-raw-option
Oct 27, 2026
Merged

fix(pcapng): resolve byte order for a raw bytes option without self._byte set (#2353)#2369
JarryShaw merged 1 commit into
mainfrom
fix/2353-pcapng-byte-order-raw-option

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — bug fix

Description

Closes #2353.

  • Fix: make settles the byte order before dispatching to the block maker — self._ctx.section.byteorder when a section is bound, else the declared order from __packet__, else the host's — the same resolution pack already uses. This covers both _make_pcapng_options (a raw bytes option) and _make_nrb_records (a raw bytes NRB record), which previously read self._byte before it was ever set outside _make_block_shb/_make_block_systemd.
  • Test: builds a raw option/record with no byte order set, re-parses each block from its own octets (not the maker's schema) and asserts the actual payload, then checks from_data rebuilds byte-exact. Pins the resolved byte order through the existing "only one of this option" guard, in a little-endian section and a big-endian section, and via __packet__={'byteorder': ...} with no section.
  • Fail-without-fix: 10 test methods, 16 failures on origin/main (all AttributeError: 'PCAPNG' object has no attribute '_byte'). Passing on the branch.
  • Note: an external PR (fix(pcapng): resolve byteorder safely in _make_pcapng_options for raw bytes options #2355) addresses the same issue by a different fallback order; this is our own implementation.

@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 27, 2026
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) 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 27, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 27, 2026
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Review: NEEDS CHANGES at 74809c3bf (Sonnet). Fix works for the common case; four things before merge.

  1. Real regression: a ctx-less, non-SHB block with __packet__=None passed explicitly used to build on origin/main; now raises TypeError: argument of type 'NoneType' is not a container or iterable. Fix is narrowing packet_byteorder(kwargs.get('__packet__')) to fall back to {} on None.
  2. Three mutants of the resolution chain survive the current tests: a host-fallback-order flip, __packet__-vs-section precedence, and the nested-__packet__-level lookup. Need one pinning test each.
  3. Two change-relative "used to" passages (the pcapng.py byteorder-fallback comment and the test module docstring) — this repo's docs: #719-style prose sweep after the #1090–#1102 defect wave #1103 convention states current behavior, not before/after history.
  4. PR body's "6 failed on origin/main" undercounts — it's a test-method count; the real failure count (subtests) is 10.

Fix in progress.

@github-actions

github-actions Bot commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Coverage: 91.58% (unit tier, Python 3.15, 3d2923880, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19601 880 2620 867 92.12%
pcapkit/corekit 2596 74 818 37 96.51%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 3029 23 1050 25 98.82%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17459 218 4600 185 98.15%
pcapkit/toolkit 914 36 334 10 95.51%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4432 1988 1018 167 51.87%

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

@JarryShaw
fix(pcapng): resolve byte order for a raw bytes option without self._…
3d29238 
…byte set (#2353)

An option or record handed to a block maker as raw bytes has its type code read
back out of those octets, in the block's byte order -- which only
`_make_block_shb` and `_make_block_systemd` set. A raw option in any other
block, and a raw record in an NRB, therefore raised `AttributeError: 'PCAPNG'
object has no attribute '_byte'`.

`make` now settles the byte order before the block maker runs, the way `pack`
settles it: the section's, else the one the packet data declares, else the
host's. A section header block still overrides it from its own Byte-Order
Magic. Settling it here covers every block maker: `_make_block_shb` and
`_make_block_systemd` settle their own, and without this the rest read an
attribute that is not there.

`__packet__` handed over as `None` degrades to no packet data, matching how
`Schema.pack` already treats it, rather than raising a `TypeError`.

Tested by a unit module: a raw option and a raw record build with no byte
order set, each block parses back from its own octets and rebuilds byte for
byte, the code resolved is pinned through the "only one of this option" guard
in a big-endian section as well as a little-endian one, a bound section
outranks a byte order the packet data declares, a byte order declared one
level up inside `__packet__` is read correctly, and `__packet__=None` resolves
to the host order instead of raising.

Fail-without-fix: 10 test methods, 16 failures on `origin/main`.
@JarryShaw
JarryShaw force-pushed the fix/2353-pcapng-byte-order-raw-option branch from 74809c3 to 3d29238 Compare October 27, 2026 07:17
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Rebased and amended 74809c3bf → 3d2923880 (6 commits onto current origin/main, clean rebase, no conflicts):

  1. __packet__=None regression fixed: packet_byteorder(kwargs.get('__packet__') or {}) so None degrades to no packet data (matching how Schema.pack already treats it) instead of raising TypeError.
  2. Added pinning tests for the three surviving mutants (host-fallback flip, section-vs-__packet__ precedence, nested-__packet__-level lookup) — each independently mutation-verified.
  3. Both change-relative "used to" passages reworded to state current behavior, matching the docs: #719-style prose sweep after the #1090–#1102 defect wave #1103 sweep convention.
  4. Commit message corrected: fail-without-fix is 10 test methods / 16 failures on origin/main, not 6.

10/10 tests pass. Fresh review round dispatched.

@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at 3d2923880 (Sonnet, round 3). All code/test fixes independently re-verified: __packet__=None regression confirmed fixed (byte-identical to main for the working case; pre-existing pack() gap in the section/SHB cases confirmed NOT a regression, unchanged on main), all 6 mutants independently re-killed, prose clean, regression set clean (830 passed vs main's 820, exact diff = this PR's tests). Only gap was the PR body's stale "6 failed" — fixed to the accurate "10 test methods, 16 failures".

All findings across all 3 rounds now addressed.

@JarryShaw JarryShaw mentioned this pull request Oct 27, 2026
Closed
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Ready to merge.

Review: GOOD TO GO (Sonnet, round 3) at 3d2923880.
CI: Required checks passed, 0 failures (only the non-blocking experimental GraalPy leg still pending).
Mergeable, 0 unresolved threads.

@JarryShaw JarryShaw added the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 27, 2026
@JarryShaw
JarryShaw merged commit 7a1dc2c into main Oct 27, 2026
62 checks passed
@JarryShaw
JarryShaw deleted the fix/2353-pcapng-byte-order-raw-option branch October 27, 2026 15:02
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 27, 2026
@JarryShaw JarryShaw removed review: good-to-go Cross-review at the current head says ready; CI state is separate review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@JarryShaw JarryShaw mentioned this pull request Oct 28, 2026
Merged
4 tasks done
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs(changelog): backfill entries for the #2312-#2407 range (#2409)
7f59b3e 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Two pairs share a
bullet, since each is the direct follow-up of the other: #2393 with
#2401, and #2405 with #2407. The diff on the entry file is purely
additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on the seven classes owning
a __proto__ registry (#2392), __proto_dest__ on the four enum registries
and Vendor.PROTO_DEST to generate it (#2403). Both PRs added the autodoc
entries, so both are rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs(changelog): backfill entries for the #2312-#2407 range (#2409)
813e9f8 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Three pairs share a
bullet, since each is the direct follow-up or counterpart of the other:
#2393 with #2401, #2405 with #2407, and #2392 with #2403. The diff on the
entry file is purely additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on each class owning a
__proto__ registry (#2392), __proto_dest__ on the enum registries and
Vendor.PROTO_DEST to generate it (#2403). The bullet names those classes
and registries rather than counting them, per the no-counted-numbers rule
for the unreleased entry. Both PRs added the autodoc entries, so both are
rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs: backfill changelog entries for the #2312-#2407 range (#2412)
27c797d 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Three pairs share a
bullet, since each is the direct follow-up or counterpart of the other:
#2393 with #2401, #2405 with #2407, and #2392 with #2403. The diff on the
entry file is purely additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on each class owning a
__proto__ registry (#2392), __proto_dest__ on the enum registries and
Vendor.PROTO_DEST to generate it (#2403). The bullet names those classes
and registries rather than counting them, per the no-counted-numbers rule
for the unreleased entry. Both PRs added the autodoc entries, so both are
rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix)

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.5

Development

Successfully merging this pull request may close these issues.

fix(pcapng): a raw bytes option in options= raises AttributeError on self._byte

1 participant

@JarryShaw