[Qiu-LingYan]

Fixes #2353. Also fixes the identical self._byte read in _make_nrb_records for raw bytes records in an NRB's records=[...], as flagged in the round-1 review.

Problem

_make_pcapng_options reads self._byte when it encounters a raw bytes item in an options=[...] list, and _make_nrb_records does the same for raw bytes items in records=[...]. Only _make_block_shb and the systemd journal entry maker ever set that attribute, so making any other block (IDB, DSB, EPB, ISB, NRB, PB, CB, ...) with a raw bytes option/record raised:

AttributeError: 'PCAPNG' object has no attribute '_byte'

Repro (fails on main):

from pcapkit.protocols.misc.pcapng import PCAPNG
PCAPNG(num=1, sct=1, ctx=None, type=0x00000001,
       block={'linktype': 1, 'snaplen': 0x40000,
              'options': [b'\x01\x00\x05\x00hello\x00\x00\x00']})

Fix

Both makers now resolve the byteorder in the same way: use the instance's _byte when set (the SHB and journal paths set it themselves), otherwise the bound section context's byteorder, otherwise sys.byteorder.

Tests

tests/protocols/misc/test_pcapng_raw_bytes_option_2353_unit.py — 6 tests / 5 subtests:

• duplicate-option check pins the resolved byte order: a raw if_name option followed by a typed if_name option raises ProtocolError only when the raw header decodes to the same code — little-endian section, big-endian section, no context, plus the _byte-first tier (instance reuse after an SHB build) and the host fallback (mock.patch.object(sys, 'byteorder', ...))
• raw-bytes build compared byte-for-byte against the equivalent typed-option build, without a context and within both byte-order sections (the raw item is encoded in the resolved byte order)
• real parse-back through a parsed SHB's section context, reading the option's actual payload (opt_comment → hello)
• byte-exact from_data(info) rebuild for both byte orders
• NRB with a raw bytes record compared against the typed build (the records maker's decoded type is only compared against the byte-order-invariant end-record sentinel, so its byte order is not externally observable)

7 failed / 1 passed on main → 6 passed + 5 subtests on this head. test_pack_keeps_instance_state_1769_unit.py + test_arp_pcapng_hwaddr_octets_unit.py pass (14 passed, 19 subtests); the 3 failures in test_capture_roundtrip_runtime.py are pre-existing on main and unrelated.

[JarryShaw]

Review: NEEDS CHANGES at 8032e7e41 (Sonnet). The production fix is correct; the tests need tightening.

1. Test 2's second block doesn't parse. It builds a little-endian raw option inside a big-endian section. On a real parse, the option doesn't even round-trip — it comes back with a warning and empty options. Encode the option in the section's own byte order.

2. The mutation check found the tests prove less than they claim. All six mutants I tried — byteorder always big, always little, ignoring the context, inverted, dropping the option, replacing it with zeros — still pass both tests. Please replace the len(data) % 4 == 0 style assertions with: the built octets compared against the equivalent typed-option build, a real parse-back that reads the option's actual payload, and a byte-exact from_data rebuild.

3. Prose. "Used to raise", "now falls back" and "must keep working" are change-relative wording the repo's test prose avoids; and the "SHB-then-block ordering still works" claim in the test docstring and PR body isn't accurate — the two instances are linked only through the shared context, not ordering.

Verified independently, and it holds:

• The fix correctly covers every non-SHB block maker that reads self._byte (IDB, DSB, EPB, ISB, NRB, PB, CB); SHB and the journal-entry maker, which already set it, are byte-identical before and after.
• The byte order it falls back to is actually correct — a raw option built this way matches the typed-option build and parses back byte-exact, in both byte orders, with and without a context — though your own tests don't currently pin this (see NameError: name 'packet' is not defined #2 above).
• No regressions across 369+68 nearby and wider tests. Fail-without-fix: 2 failed on origin/main; 2 passed on the head.

One more site with the identical bug, found while reviewing — worth folding into this PR or its own: _make_nrb_records has the same self._byte read for a raw bytes record in an NRB's records=[...], same crash, same one-line fix.

[Qiu-LingYan]

Thanks for the thorough review — all four points addressed in 1eaaf77:

1. NRB: folded into this PR — _make_nrb_records now resolves the byteorder the same way (getattr fallback → section context → sys.byteorder).
2. Test 2 byte order: the raw option is now encoded big-endian (\x00\x01\x00\x05...) inside the big-endian section; verified it round-trips and reads back the payload.
3. Stronger assertions: the tests now (a) compare the raw-bytes build byte-for-byte against the equivalent typed-option build (no context, little-endian section, big-endian section), (b) parse the built block back through a real parsed-SHB section context and read the option payload (opt_comment → hello), and (c) check from_data(info) rebuilds the octets byte-exact — all in both byte orders. Also added a raw-bytes NRB record test. All 4 fail on main, pass on this head.
4. Prose: change-relative wording removed from the test docstrings; the inaccurate "SHB-then-block ordering" claim is gone — the docstrings now describe the fallback chain, and the PR body was rewritten.

[JarryShaw]

Review: NEEDS CHANGES at 1eaaf77c4 (Sonnet, round 2). Thanks for the fix to _make_nrb_records and the byte-order test — the production fix is correct. One more gap, though: 4 of the 6 round-1 mutants (always big-endian, always little-endian, ignoring the context, inverted) still survive the new tests on both makers.

On the options maker, this is reachable: a raw if_name option followed by a typed if_name option raises ProtocolError ("must be only one") only when both decode to the same code, so that's a test that actually observes the resolved byte order. Could you add it, in a little-endian section, a big-endian section, and with no context?

On the records maker, I think this one's genuinely hard to observe from the outside — the decoded type is only ever compared against the all-zero end-record sentinel, which is byte-order invariant either way. If there's no behavioral difference to pin there, that's fine; just flagging it rather than asking for an impossible test.

Two more things, worth folding into the same edit:

• The no-context tests hard-code little-endian bytes, so they'd fail on a big-endian host (I verified by simulating one). Deriving them from sys.byteorder would make that portable.
• The module docstring still narrates the pre-fix defect in the past tense ("read self._byte … although only the SHB … ever set"), rather than stating the current behaviour.

Everything else held up: no regression against the broader pcapng/docstring/tier-guard suite (230+193 passed, including a clean merge onto current main), isort/mypy/pylint unchanged from main.

[Qiu-LingYan]

All three points addressed (tests-only commit, production code untouched):

1. Byte-order-observing test: added test_duplicate_check_observes_resolved_byte_order — a raw if_name option followed by a typed if_name option is rejected with ProtocolError only when the raw header decodes to the same code, asserted in a little-endian section, a big-endian section and without a context. Self-checked with an always-big-endian mutant: it fails this test (and the big-endian roundtrip) while passing the round-2 version.
2. Records maker: agreed — the decoded record type is only compared against the all-zero end-record sentinel, which is byte-order invariant, so there is no external behavior to pin. The module docstring now states this explicitly, and the NRB test keeps pinning the produced octets against the typed build.
3. Portability: the no-context option and NRB record tests now derive their raw bytes from sys.byteorder instead of hard-coded little-endian bytes.
4. Docstring: rewritten to state the current behaviour (fallback chain) rather than narrating the pre-fix defect.

On main the suite is 7 failed / 1 passed; on this head 5 passed + 3 subtests. test_pack_keeps_instance_state_1769_unit.py still passes.

[JarryShaw]

Review: GOOD TO GO at 6706c7b1e (Sonnet, round 3). All round-2 findings independently re-verified: all 4 previously-surviving options-maker mutants now killed (confirmed on a simulated big-endian host too); the _make_nrb_records byte-order-invariance claim holds structurally and empirically (65,799 cases, 0 differences — these mutants are equivalent, not weakly tested); portability fix confirmed; docstring now timeless; fail-without-fix numbers exact (7 failed/1 passed → 5 passed/3 subtests). Full regression clean (0 failed across protocols/foundation/project/etc.), clean merge onto current main.

Nits, non-blocking: the _byte-first tier and no-context host-fallback are each unpinned on a single host (fixable with a mock.patch.object(sys, 'byteorder', ...) pair); the PR body is stale (says "4 tests" where it's now 5+3 subtests) and off-template; file location differs from this repo's tests/protocols/misc/ convention for pcapng tests.

For context: our own #2369 (also closing #2353) takes a different approach — settles byte order once in make() rather than per-maker — which incidentally also fixes .byteorder on a made non-SHB block with no context, something this PR doesn't touch. Not a blocker here, just noting the scope difference since both PRs target the same issue.

[Qiu-LingYan]

Nits addressed in 7e0a4a23 (tests-only):

• Tier pins: added test_byte_order_tiers_are_pinned — the _byte-first tier via instance reuse after an SHB build (big-endian), and the host fallback via mock.patch.object(sys, 'byteorder', 'big') on a fresh context-less instance.
• PR body: updated — correct test count (6 tests / 5 subtests) and location (tests/protocols/misc/).
• Location: test file moved to tests/protocols/misc/ per repo convention.

Also noted your #2369 — settling the byte order once in make() is the cleaner architecture, and covering .byteorder on made non-SHB blocks is a fair scope advantage. Happy to close this in favor of #2369 if you'd rather take that route; otherwise this is ready as-is.

[JarryShaw]

Appreciated — thanks for addressing all three rounds thoroughly, and for the generous offer. Taking you up on it: #2369 (our own fix for #2353) is already ready-to-merge and covers a bit more ground (the .byteorder fix on made non-SHB blocks you noted), so closing this one in its favor rather than carrying two fixes for the same issue. Nothing wrong with this PR — it was solid work, genuinely GOOD TO GO on its own merits.