fix: strip a systemd journal entry's own block padding, not its data

[JarryShaw]

Please follow the guide below

• You will be asked some questions, please read them carefully and answer honestly

• Put an x into all the boxes [ ] relevant to your pull request (like that [x])

• Use Preview tab to see how your pull request will actually look like

• 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 under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — 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?

Tick the commit type your subject line carries.

• fix — corrects a defect
• feat — adds a feature
• perf — changes performance, not behaviour
• refactor — changes neither behaviour nor performance
• test — tests only
• docs — documentation only
• ci — workflows or build tooling
• chore — anything else

---

Description of your pull request and other information

Chose the tolerant fix (strip the padding), not ProtocolError: a binary field's value
is read by its own 64-bit length prefix, never by scanning for a line ending, so the
defect is unreachable there. Block alignment padding is always 0-3 octets, so at most
the trailing 3 octets of a terminator-less line are stripped as padding, with a
SchemaWarning — and only when the raw, unstripped line itself ends in NUL. A real
trailing newline, or any other raw last octet (ordinary ASCII whitespace included),
proves the true padding is zero and leaves the line untouched.

Cross-review found two defects in the first cut. First, it gated on raw_line for the
missing newline but stripped NULs off line (raw_line.strip()), exposing real data
NULs as padding whenever the entry's actual last octet was ASCII whitespace — wrong on
3/12 simulated cases, fixed to 0/12. Second, once that gate requires raw_line to end
in NUL, bytes.strip() can never remove it, so the strip loop always ran and the
if pad_octets: guard around the warn+assign was dead code; dropped.

Measured: b'MESSAGE=hello' + b'\x00\x00\x00' (no terminator) returned
MESSAGE == 'hello\x00\x00\x00' before any fix, 'hello' plus a SchemaWarning after.

tests/protocols/misc/test_pcapng_unit.py (93/1762) and
tests/integration/test_pcapng_end_to_end.py (6/1/25) both green, cross-checked under
python -m unittest. Coverage on the changed module is 100% (564 statements, 84
branches, 0 missed). No new mypy or pylint findings against main. Zero of the 26
SchemaWarnings raised across all 23 example fixtures come from this code path.

Closes #794

[JarryShaw]

Cross-review at efd38e875 (haiku, a different model from the author): NEEDS CHANGES — one blocking defect, and it is a mirror of the defect this PR exists to fix.

The branch gates on raw_line but counts NULs off line, which is raw_line.strip(). So when the entry's final octet is ASCII whitespace, the padding is provably zero — padding is NUL, and if the last octet were NUL then strip() could remove nothing on the right — yet it strips 1-3 real data octets and emits a SchemaWarning asserting they were "the block's own alignment padding". Verified myself on 12-octet, 4-aligned bodies:

MESSAGE=hi\x00 + SPACE  -> {'MESSAGE': 'hi'}  + SchemaWarning     (base: 'hi\x00', no warning)
MESSAGE=hi\x00 + CR     -> {'MESSAGE': 'hi'}  + SchemaWarning     (base: 'hi\x00', no warning)
MESSAGE=\x00\x00\x00 + TAB -> {'MESSAGE': ''}  + SchemaWarning     (base: '\x00\x00\x00', no warning)

That is a 100% false-positive rate on that path, not a partial one, and a strict regression against base for those inputs. It also violates the docstring's own rule — these are not the trailing octets of the entry.

The fix is one condition, which the reviewer simulated over 12 cases (PR wrong on 3/12, gated version wrong on 0/12, identical on all nine the PR already gets right):

if not raw_line.endswith(b'\n') and raw_line.endswith(b'\x00'):

It also refuted row 4 of the boundary table I published. I reported b'MESSAGE=hello' + 5 NUL → 'hello\x00\x00'. That input is 18 octets, and a block body is 4-aligned, so it cannot occur — my hand-built entry bypassed the writer's padding. The legal equivalent is b'MESSAGE=hel' + 5 NUL (16 octets), which gives 'hel\x00\x00'. The substantive point stands — clip at 3, leave the rest — but my row was measuring an impossible input.

What it confirmed, with a stronger argument than the author's. Binary fields really are unreachable, and the structural reason is better: io.BytesIO.readline() returns a line without \n only at EOF, so any terminator-less line is the last in the buffer and any binary field it names must fail its own read(8). It also worked the arithmetic on "at most 3" and found the congruence carries zero information about the true pad — all four candidates are self-consistent — so 3 is the tight bound, not a guess. Counts all reproduced (92/1757, Ran 92 OK, 6/1/25, 100% coverage with 0 missed branches), #699's guard intact, and zero new SchemaWarnings across all 23 real fixtures.

Two prose items also routed: the docstring's "is not reached at all" clause is false (measured — the terminator check is reached and passes), and the PR body's "each new padding-count subtest shown failing" needs "each non-zero", since pad_octets=0 passes on base by design.

[JarryShaw]

Revised to 5226a0398, rebased onto current main (2d1c8675d). The blocking defect is fixed and I verified it myself:

-- the three whitespace cases: real data must SURVIVE, no warning --
  hi\x00 + SPACE  -> {'MESSAGE': 'hi\x00'}          SchemaWarning=[]
  hi\x00 + CR     -> {'MESSAGE': 'hi\x00'}          SchemaWarning=[]
  \x00x3 + TAB    -> {'MESSAGE': '\x00\x00\x00'}    SchemaWarning=[]

-- and the real defect is still fixed --
  hello + 3 NUL              -> {'MESSAGE': 'hello'}   SchemaWarning=['SchemaWarning']
  terminated, value ends NUL -> {'MESSAGE': 'hi\x00'}  SchemaWarning=[]

So the gate not raw_line.endswith(b'\n') and raw_line.endswith(b'\x00') does exactly what it should: padding can only be NUL, so a non-NUL final octet proves the true padding is zero and nothing is stripped. The worker rebuilt the 12-case simulation independently against both real implementations rather than trusting my numbers, and got the same 3/12 → 0/12.

One claim in this PR's body had passed through three hands unverified, so I derived it. The reviewer found that this fix also repairs the rebuild; I relayed it; the worker put it in the body and said honestly that it had taken my word rather than deriving it, since the writer path is outside what it owned. Measured on both trees:

BASE (main)        parsed .data -> {'MESSAGE': 'hello\x00\x00\x00'}
                   rebuild      -> b'MESSAGE=hello\x00\x00\x00\n'
PR 5226a0398       parsed .data -> {'MESSAGE': 'hello'}
                   rebuild      -> b'MESSAGE=hello\n'

Confirmed. On base the alignment padding is baked permanently into the value, so a parse-then-write round trip grows the entry by three octets every time; the fix makes the rebuild match the well-formed form. That is a genuine second benefit and the body's claim stands — but it was asserted by one agent, relayed by me, and published by another without anyone measuring it, which is the chain worth not repeating.

Both prose items also landed: the "is not reached at all" clause is corrected (verified directly — BIN\n + 2-NUL value + \n + 1 padding octet gives entries[0]['BIN'] == b'\x00\x00' with zero SchemaWarnings, so the check ran and passed silently), and the imprecise "reachable only through this text-field path" is replaced with the stronger EOF argument. Body corrected to "each non-zero padding-count subtest".

New test test_a_terminator_less_line_whose_raw_tail_is_whitespace_has_zero_padding (SPACE/CR/TAB subtests) shown failing pre-amend with 3 failures. Counts: 92 passed / 1 skipped / 1760 subtests under pytest, 93 tests OK under unittest — consistent, since unittest counts the new method and pytest's total was already 92. Integration 6/1/25. 26 SchemaWarnings across all 23 fixtures, 0 from this code path. mypy and pylint identical to base.

Re-reviewing at the new head before I set a verdict.

[JarryShaw]

Delta re-check at 5226a0398 (haiku, resumed): NEEDS CHANGES — the blocking whitespace defect is fixed and verified, but the amend introduced a new one, and it also refuted a number I relayed.

The new defect: if pad_octets: at :1928 is dead code. The gate requires raw_line.endswith(b'\x00'), and bytes.strip() cannot remove a trailing NUL, so line ends in that same NUL and the while always runs at least once. Verified independently:

reach the gate: 2801   of which pad_octets == 0: 0
-> `if pad_octets:` is DEAD (always true)

(The reviewer's sweep found 3706 gate-reaching inputs to my 2801 — different alphabet and length bounds, same zero.) That dead arm is exactly the partial branch the report names:

pcapkit/protocols/schema/misc/pcapng.py   565  0  86  1   99%   1928->1935

Previous head efd38e875 measured 565 0 86 0 100%, so the amend caused the drop — and the PR body still asserts 100%. Dropping the guard is the better fix than correcting the prose: the pad_octets == 0 subtest now exercises the gate, so the guard has no remaining purpose.

It refuted my own relay of the counts. I wrote that pytest gives 92 and unittest 93, "the counts differ legitimately". Wrong — the previous head was 92/92, this one is 93/93, and adding a method raises both. The PR body's own figure was right; the error was mine.

What it confirmed by attacking the new two-condition gate, wider than the test's table: vtab \x0b and formfeed \x0c tails are also handled correctly (bytes.strip() removes those too, and the new test covers only space/CR/tab) — not a separate code path, since the gate is a positive test on NUL rather than an enumeration, so the table is adequate but two rows would cost nothing. A NUL tail preceded by whitespace works (b'MESSAGE=h \x00\x00' → 'h ', interior whitespace intact). #699's guard still takes an all-NUL final line first, at 1/2/3/4/6 NULs. Five CR-LF shapes correct, including a bare CR with no LF. And the binary-field values are byte-identical to base across all 8 constructions.

One irreducible cost it re-confirmed rather than reported as new: a NUL tail that is genuinely value data with a true pad of 0 — b'MESSAGE=ab\x00\x00' → 'ab' — cannot be separated by any gate, since all four p ∈ {0,1,2,3} are congruence-consistent with len(entry) ≡ 0 mod 4. Documented in the docstring; not a new issue.

It also verified the merge state rather than assuming it: 5226a0398^ byte-equals 2d1c8675d, fast-forwardable, and the apptype churn in git diff efd38e875 5226a0398 is rebase carry-over from main, not this PR's change.

Both prose fixes are accurate, with one unflagged exception worth a nit: the terminator check is not reached when the length prefix was over-declared and clamped (:1948 if not clamped:), where padding can land inside a binary value — pre-existing, warned, and covered by the clamping paragraph earlier in the same docstring.

Routing the dead-branch fix and the body's coverage figure.

[JarryShaw]

Revised to 5777a9348. The dead guard is gone and I verified the coverage figure myself this time — the worker corrected my projection and was right:

$ git diff 5226a0398 5777a9348 -- pcapkit/
-                    if pad_octets:
-                        warn(f'PCAP-NG: [systemd Journal Export] entry field {line!r} '
+                    warn(f'PCAP-NG: [systemd Journal Export] entry field {line!r} '

pcapkit/protocols/schema/misc/pcapng.py   564   0   84   0   100%
tests/protocols/misc/test_pcapng_unit.py  93 passed, 1762 subtests passed

I had told it to confirm coverage "returns to 565 0 86 0 100%". That was wrong. Removing a statement and its branch point drops both counts, so the real figure is 564 / 84, not 565 / 86. It reported what it measured rather than forcing my number, and said so — the right instinct, since my projection was an assumption dressed as an expectation.

It also declined to reconcile the body's counts to my retracted relay about 92-vs-93, keeping its own measured 93/1762. Also correct. For the record the difference is fixture-dependent: with all 23 captures present that module gives 93 passed, and without one of them 92 passed / 1 skipped — my run had them copied in, so I see 93.

The removed guard now carries a comment explaining why it was unreachable, which is better than silently deleting it: the outer gate requires raw_line to end in NUL, and bytes.strip() cannot remove a trailing NUL, so line ends in one too and the loop always runs at least once.

Two extra subtest rows added for vtab \x0b and formfeed \x0c, so the table now matches the full bytes.strip() set rather than just space/CR/tab. Its 14-case extended simulation is 0/14 wrong. Fixture sweep re-confirmed: 26 SchemaWarnings across all 23 fixtures, 0 from this path. mypy unchanged at the one pre-existing :682 finding; pylint's 397-finding set identical by diff.

Re-review dispatched at this head before I set a verdict.

[JarryShaw]

Delta re-check at 5777a9348 (haiku, resumed): GOOD TO GO. Both of its findings are fixed and it could not break the result.

The strongest evidence is that it brute-forced the real parser rather than simulating it. 4-aligned bodies over the alphabet {a, \x00, SP, TAB, CR, \v, \f, \n} at lengths 1–5:

66,429 bodies parsed
39,156 "has no terminating newline" warnings emitted
     0 reporting `last 0 NUL octet(s)`

So the now-unconditional warn can never report a zero count, and removing the guard changes no observable behaviour — which is the thing a coverage number alone could not establish.

It also checked the case that would have meant the gate shadowed #699's guard, and it does not: bodies of 4 and 8 bare NULs, and a field followed by a 1-NUL, 3-NUL or SP+2-NUL line, all give 0 strip warnings with values intact, because if not line.strip(b'\x00'): break at :1889 fires first. A strip warning on any of those would have been a regression.

And the two new rows earn their place. Reverting the gate to its single-condition form gave FAILED (failures=5) with ids (tail='space'), (tail='CR'), (tail='tab'), (tail='vtab'), (tail='formfeed') — so all five genuinely exercise the fix, not just the original three.

Coverage confirms my corrected arithmetic, which I had got wrong the first time: dropping if pad_octets: removes one statement (565→564) and one branch point, i.e. two arcs (86→84). Measured 564 0 84 0 100%, and it explicitly declined to reconcile to my stale 565/86 projection. I measured the same figure independently.

Everything else re-confirmed: 93 / 1762 under both runners, integration 6/1/25, 26 SchemaWarnings across all 23 fixtures with 0 from this path, mypy at the one pre-existing :682 finding, pylint's 22 rule codes identical with no message on any added line. Merge state verified by sha comparison rather than assumed — 5777a9348^ byte-equals 2d1c8675d, fast-forwardable.

Two residuals, both previously flagged and neither new: the irreducible ambiguity where a terminator-less value genuinely ending in ≤3 NULs on a 4-aligned entry loses them, which no gate can separate since all four padding counts are congruence-consistent; and the docstring's "always reached" not flagging the if not clamped: path, which the clamping paragraph earlier in the same docstring already covers.

CI 25✅ / 3⏭ / 0❌ with 2 in flight. Setting review: good-to-go — unpublished and unmerged, yours to take. Closes #794.