fix(pcapng): walk the journal entry instead of splitting it on b'\n\n'

[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• make test passes, and a test case covers the change
• Added a changelog entry — N/A — changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• fix — corrects a defect

---

Description of your pull request and other information

Fixes #723.

First review caught a regression against #722: the malformed flag this rewrite added broke
the outer per-entry loop, so a bad terminator ended the whole block, not just its entry --
silently dropping every well-formed entry behind it, contradicting the "nothing of #722's is
lost" claim this PR originally made. Fixed by dropping malformed entirely (its :1832 twin
was already dead); the existing per-entry break is now the only effect. Alternative considered
and rejected as the default: abort the rest of the block on any corruption, as deliberate
hardening -- open for the maintainer to reconsider.

Also fixes an independent alignment gap: a trailing separator landing on the block's last octet
was indistinguishable from EOF and swallowed, so a rebuild wrote length one octet short.

Tests: tests/protocols/misc/test_pcapng_unit.py, 86 passed, 1753 subtests.
Coverage: pcapng.py 100% stmts/branches (551/78).

[JarryShaw]

❌ NEEDS CHANGES @ d23bffa98 — the new malformed flag breaks the outer loop, so #722's per-entry terminator check now ends the whole block and silently drops well-formed entries that main recovers, while its warning still says "ending the entry".

[JarryShaw]

❌ NEEDS CHANGES @ d23bffa98 — the new malformed flag breaks the outer loop, so #722's per-entry terminator check now ends the whole block and silently drops well-formed entries that main recovers, while its warning still says "ending the entry".

First independent review, different model from the author. Everything below is evidence I obtained myself.

claim	verdict	evidence
#722 fully preserved	coverage ✅ / semantics ❌	#722's own test file against #728's library → 81 passed, 1753 subtests; reverting only the library to daa953d1b fails only #723's two new tests (2 != 1, 3 != 1). All 5 of #722's tests present with byte-identical assertion bodies. But the terminator check's effect grew — below.
#723 A + B fixed	✅	binary value b'x\n\ny\n\nz' and the 2570-octet case (pack('<Q', 2570) == b'\n\n' + 6 NULs) both parse whole and rebuild byte-identical; main mangles both.
separator vs pad vs EOF	✅	invariant holds — entries = separators read + 1 — so the rebuild is byte-identical for every well-formed input I tried, including b'\n', b'\n\n', and 2 and 3 trailing separators, all of which main gets wrong.
bounded on bad input	✅	~40 crafted inputs: no escaped struct.error / IndexError / OverflowError / UnicodeDecodeError, no over-read, no hang (readline() consumes ≥1 octet while tell() < total, so the walk is bounded by len(entry)).
OK ⇒ byte-identity	✅	examples/generators/options.py:1718 if octets != again: MISMATCH. Journal case: OK, warnings=(), 100 octets — identical on main.
breaking label	not needed	found no behaviour change for well-formed input beyond the fix itself; every difference is on malformed input. fix is right.

Required change. pcapkit/protocols/schema/misc/pcapng.py:1857 sets malformed = True; :1861 breaks the outer per-entry loop:

b'DATA\n' + struct.pack('<Q', 3) + b'abcQ' + b'\n\n' + b'GOOD=1\n'
main bf57b4542 -> [{'DATA': b'abc'}, {'GOOD': '1'}]
#728 d23bffa98 -> [{'DATA': b'abc'}]          # GOOD=1 gone; warning text unchanged

The reader's position is well defined after a bad terminator, so bailing is a policy choice, not a necessity — and it is the shape this docstring's own #678 note argues against ("aborted the whole extraction rather than this one entry"), one level down. Either drop malformed on that path, or keep it and fix the warning, which says "ending the entry" while ending the walk — and declare it, because the PR body says #722's check is "unchanged" and its hunks "byte-identical". Two further points on the same flag: :1832's malformed = True is dead (read(8) returns <8 only at EOF, so the tell() >= total guard already fires; deleting it leaves 84 passed, 1753 subtests, unchanged), and no test pins the entry count after a bad terminator, so the new abort semantics are unverified — the 2 subtests that notice the flag only pin a warning count.

Minor, not blocking. separator = bool(raw_line) (:1816) counts a trailing whitespace-only line with no newline as a separator, so b'ABC=12\n ' rebuilds as b'ABC=12\n\n' — same length, so the length % 4 guard cannot catch it, where main raised invalid length: 19. Loud → silent, on malformed input only.

Counts (repo venv 3.14.7, pcapkit.__file__ proved inside my worktree, fixtures generated): branch d23bffa98 → 90 passed, 2113 subtests; merged into bf57b4542 with --no-ff (exit 0, no conflicts) → 90 passed, 2113 subtests, over tests/protocols/misc/test_pcapng_unit.py + tests/protocols/test_option_roundtrip_unit.py. EXPECTED_FAILURES blob unchanged vs main; no key mentions journal/systemd.

❌ NEEDS CHANGES @ d23bffa98 — the new malformed flag breaks the outer loop, so #722's per-entry terminator check now ends the whole block and silently drops well-formed entries that main recovers, while its warning still says "ending the entry".

[JarryShaw]

Fixed @ dcd480891. The malformed flag broke the outer per-entry loop, not just the field
loop inside it -- removed entirely, restoring #722's per-entry scope. Its :1832 twin was dead
(EOF-only); confirmed and removed too. Warning text ("ending the entry") is now accurate again,
since that's the only thing a bad terminator does.

Added two tests pinning behaviour relative to main: one with no separator between the bad
entry and the next (2 entries, matches main exactly), one with a real \n\n separator (4
entries -- main's split produces 2; the extra two are empty entries from how the walk resyncs
on the separator after a corrupted terminator, documented in the test, nothing dropped).

Abort-the-block-on-any-corruption was considered as deliberate hardening and rejected as the
default -- happy to revisit if that's preferred.

86 passed, 1753 subtests branch-only; 92 passed, 2113 subtests merged into bf57b4542 (--no-ff,
clean); pcapng.py 100% stmts/branches (551/78) both ways.

[JarryShaw]

✅ GOOD TO MERGE @ dcd480891 — good to merge, one judgement call for you: the two empty entries are cosmetic, not loss (0 silent losses across 656 adversarial cases), and main is the broken tree — it warns on 218 of 300 generated well-formed entries where #728 warns on none.

[JarryShaw]

✅ GOOD TO MERGE @ dcd480891 — good to merge, one judgement call for you: the two empty entries are cosmetic, not loss (0 silent losses across 656 adversarial cases), and main is the broken tree — it warns on 218 of 300 generated well-formed entries where #728 warns on none.

Second review; supersedes the ❌ NEEDS CHANGES on d23bffa98. Differential execution against main bf57b4542, each run asserting its own pcapkit.__file__.

load-bearing claim	verdict	evidence I obtained
data-loss regression fixed	✅	main silently drops GOOD=1, K=v and B=2 on three distinct shapes #728 recovers; over 656 mutation/insertion/truncation/random cases, 0 where #728 lacks a pair main produced without warning
is main the right standard?	❌ no	300 generated well-formed entries: #728 0 SchemaWarnings, main 218 — it fabricates field names from length prefixes and cuts binary values at \n\n. #728 never read fewer fields; more in 170
does removing the flag re-open anything?	✅	separator=True ⇒ ≥1 octet consumed, so ≤ total+1 iterations; every read is BytesIO-bounded and available is now measured against the whole buffer; exit needs tell>=total, so no octet goes unwalked
was :1832 genuinely dead?	✅	instrumented that break: fired 230×, and tell==total and not separator held 230/230 — the outer guard already fires there
do #722's behaviours hold?	✅	all five tests' inputs plus both sweeps byte-identical; only declared ∈ (0,1,2) diverge, and additively
loosened warning assertion	✅ nit	it accommodates a genuine second warning and pins the first's content, so it was necessary; it could still pin exact counts (2 for 0/1, 1 for 2/4/5) rather than "any further short-prefix"
outer abort not shipped	✅ right call	aborting would reproduce main's data loss by design; per-record degradation is this module's own precedent (#678, #431, #722)

Counts reproduced independently: branch-only 86 passed / 1753 subtests, pcapng.py 100% statements + 100% branches (551/78); --no-ff merge into bf57b4542 92 passed / 2113 subtests. Both new tests fail on d23bffa98 with exactly 1 != 2 and 1 != 4.

Round-trip — and the rebuild is data-driven (_make_block_systemd joins entries with b'\n'), so empty entries do reach the octets: #728 is byte-exact against the original entry on 7 cases where main is not, never worse, and parse→rebuild→parse is idempotent on all 28 cases where main fails 3. The 2-vs-4 measurement reproduces exactly.

Caveats, none blocking: (a) no round-trip test exercises the rebuild path with empty entries present; (b) #728 walks into the block's NUL padding in more situations, so padding adjacent to non-NUL garbage can synthesise an empty-key junk field main skipped — malformed input only, unreachable when the last field is newline-terminated; (c) the post_process note's "used to be swallowed … a rebuild lost that one octet" describes this PR's own earlier draft, not main — main also yields 2 entries for b'ABC=12\n\n', so a later reader will mis-attribute that bug.

✅ GOOD TO MERGE @ dcd480891 — good to merge, one judgement call for you: suppressing the empty entry that follows a corruption-induced break would match main's tidier count, but it needs "we broke early" state — precisely the bookkeeping the malformed flag got wrong. Your call, and not a defect either way.