fix(corekit): pad a short field read on the tail, not the head (#604)

[JarryShaw]

Fixes #604

The defect, and where it lives now

FieldBase.unpack zero-fills a short read with rjust. The issue cites
pcapkit/corekit/fields/field.py:244; on the current main the line has moved to
pcapkit/corekit/fields/field.py:506 (#593's budget work landed above it):

value = struct.unpack(self.template, buffer[:length].rjust(length, b'\x00'))[0]

rjust puts the zeros at the front, which asserts that the octets never read
were the leading ones. A short read asserts the opposite: the buffer ran out, so
what is missing is whatever came after what was read. The zeros belong at the
end. It is now ljust.

ljust is correct for both byte orders — confirmed

The comment on #604 is right that this is not a little-endian-only defect, and the
fix is therefore not byte-order-conditional. Measured on origin/main
(PADS WITH: rjust, tree identity asserted before import) and on this branch:

case	rjust (before)	ljust (after)	truth with unread octets zeroed
little-endian, 1 of 4 octets of 120 (0x78)	2013265920	120	120
big-endian, 3 of 4 octets of 0x01020304	0x10203	0x1020300	0x1020300
big-endian, 1 of 2 octets of 0x0102	0x1	0x100	0x100
little-endian, 1 of 2 octets of 0x0102	0x200	0x2	0x2
big-endian, 5 of 8 octets of 0x0102030405060708	0x102030405	0x102030405000000	0x102030405000000
little-endian, 5 of 8 octets	0x405060708000000	0x405060708	0x405060708

Both directions are wrong under rjust; they merely fail opposite ways. The
big-endian half scales the value down, so it passes a sanity check far more
easily than the inflated little-endian half — which is why only the loud one was
ever reported. A full read is byte-identical before and after, at every width and
both orders, because the padding is only ever consulted when the buffer falls
short.

The MemoryError chain does resolve — with a caveat worth stating

The #594 worker's report was reported-but-unverified going in. It is now
verified, with one correction.

Truncating examples/captures/dhcp_little_endian.pcapng (1772 octets) leaves a
one-octet read of a little-endian 32-bit PCAP-NG block length. Read out of the
failing frame's own locals on the unfixed tree:

truncated to	declared block length under rjust	passed to
161	2013265920 (0x78000000, 1.88 GiB)	self._file.read()
641	2214592512 (0x84000000, 2.06 GiB)	self._file.read()
1389	2214592512 (0x84000000, 2.06 GiB)	self._file.read()

Under a 1 GiB RLIMIT_AS, all three raise MemoryError at exactly
pcapkit/protocols/protocol.py:1016 (return self._file.read(*args, **kwargs)),
as reported. On this branch all three instead end in the ordinary, already-handled
ValueError: read length must be non-negative or -1 at
pcapkit/protocols/schema/schema.py:857, and the same reads report 120 and 132.

The caveat: whether the symptom is a MemoryError depends on how much
address space the process can get. At a 4 GiB cap the ~1.9–2.1 GiB allocation
succeeds and the parse goes on to fail with the same ValueError it now fails with
directly — so on an unconstrained host the observable bug is a ~1.2 million-fold
allocation amplification from a 1772-octet file rather than a crash. The 161
figure in the report is real at a 1 GiB cap; a cross-check at a 2 GiB cap saw
ValueError at 161 and MemoryError only at 641 and 1389, which is consistent
with the sizes above rather than contradicting them.

A truncated capture must still parse — verified both ways

This changes what a truncated field reports, which is the point. It must not
change whether a capture parses (#431; PR #571 was declined for breaking that,
and #593's budget is built around preserving it). Both sides swept against the
same base (a2be2cc1a), fixtures regenerated for each, tree identity and padding
side asserted at import:

• 20 example captures, every full parse identical — 1594 frames both ways.
• 232 outcomes compared (full + truncation points), 231 byte-identical.
• Truncated-variant exception identity unchanged in aggregate: 209 ValueError
  plus 1 struct.error on each side; the same 2 truncated variants still parse.
• The one difference is which ValueError one already-failing case raises:
  many_interfaces.pcapng cut to 5222 octets moves from
  ValueError: 393216 is not a valid BlockType (corekit/fields/numbers.py:513)
  to ValueError: read length must be non-negative or -1
  (protocols/schema/schema.py:857). Both are ordinary parse failures at a
  truncation point, neither is a crash, and it failed before the change too.

Tests

FieldBaseShortReadPaddingSideTests, appended to
tests/corekit/test_fields_field.py (extended additively — #593's structure is
untouched): 10 cases, 133 subtests, covering both byte orders at 2, 4 and 8 octets
at every truncation point, the two figures #604 reports as literals, the
value-preserving property stated as a property rather than a table, both failure
directions as inequalities, a signed field, a byte-string field, and an
unpack-then-pack cycle.

Run against the unfixed tree first (a pristine git archive export, PADS WITH: rjust asserted), then against this branch:

# unfixed tree
85 failed, 8 passed, 25 deselected, 1 warning, 50 subtests passed in 10.35s

# this branch
10 passed, 25 deselected, 1 warning, 133 subtests passed in 8.70s

8 of the 10 cases fail without the fix. The 2 that pass on both trees are
deliberate guard rails and must not move:

• test_a_full_read_is_untouched_at_every_width_and_order — a complete field
  cannot change.
• test_an_entirely_empty_buffer_still_reads_as_zero — an empty buffer pads to
  all zeros either way, which is the OptionField.unpack never returns for a well-formed HOPOPT header with an SMF_DPD option #431 end-of-option-list / Pad1 behaviour
  the option and list loops depend on.

Scoped suite runs (never the whole tree — this host has no swap):

tests/corekit/            144 passed, 328 subtests passed          exit 0
tests/protocols/test_option_roundtrip_unit.py
                            6 passed, 358 subtests passed          exit 0
tests/integration/         90 passed, 2 skipped, 115 subtests      exit 0
tests/protocols/          615 passed, 1467 subtests, 2 failed      exit 1  (see below)

EXPECTED_FAILURES in tests/protocols/test_option_roundtrip_unit.py is
unaffected — no entry flips, so that file is not touched.

Coverage of pcapkit/corekit/fields/field.py is 84% before and after
(126 statements, 16 missed, 26 branches, 5 partial, identical missing-line sets
modulo the comment lines added). That is honest rather than a win: line 506 was
already executed by the existing suite, so a statement counter cannot see this
change. What rises is behavioural coverage — 195 → 328 subtests in
tests/corekit/, i.e. 133 new assertions over byte order × width × truncation
point, which is exactly the axis statement coverage is blind to.

One test left failing, deliberately — needs the owner of that file

tests/protocols/transport/test_tcp_udp_unit.py::TCPUDPUnitTests::test_a_truncated_option_still_parses_its_declared_length
fails on this branch, in both its subtests (declared_length=12 and 32). It
pins the old padding side:

self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing)   # line 1349

Under ljust the correct expectation is trailing + b'\x00' * zeroes, and the
docstring at lines 1307–1310 ("left-pads … four zero octets followed by the six
real ones") needs the same inversion. I have not made that change: that file is
concurrently owned by another worker in this batch, and editing it risked clobbering
their work. It is a one-line assertion plus a prose sentence. The sibling case in
tests/protocols/internet/test_ipv4_unit.py is the same defect and is fixed here,
since that file was not contended.

Note line 1350, self.assertEqual(bytes(proto.__header__), raw), passes — the pack
path is byte-preserving and is not affected.

Found while here, deliberately not fixed

• pcapkit/corekit/io.py:328 — SeekableReader.truncate does
  io.BytesIO(temp.rjust(size, b'\x00')). Same family of defect: _buffer is
  front-anchored to _buffer_set, so growing it should append capacity at the end,
  and rjust instead shoves the cached content to the end while _buffer_set is
  left alone, corrupting the index-to-offset mapping. Reproduced: after reading
  b'abcd' and truncate(8), a seek(0) read returns b'\x00\x00\x00e'.
  tests/corekit/test_io.py:131-150 only checks truncate()'s return value, never
  its content, so nothing catches it. Out of scope for FieldBase.unpack pads a short read with rjust regardless of byte order, silently corrupting little-endian values #604 and it is not a
  blind rjust→ljust swap — SeekableReader also inherits io.BufferedReader's
  own C-level buffering, so the fix wants its own investigation. Worth its own
  issue.
• Raw ValueError from library code. Every truncated capture in the sweep ends
  in ValueError: read length must be non-negative or -1 raised from
  pcapkit/protocols/schema/schema.py:826 / :857, not from
  pcapkit.utilities.exceptions. Pre-existing, unchanged by this PR, and contrary
  to the in-library exception convention.
• Partial reads of a 2-octet option type field. Four OptionField selectors
  use a 2-octet type (hip.py:313, sctp.py:427, sctp.py:232, mh.py:814, and
  PCAP-NG's Option.type, the only one with eool wired up). When a capture ends
  mid-type-field exactly one octet survives, which is a genuinely partial read
  rather than an empty one, so it decodes to 0 under neither padding side unless the
  surviving octet is itself 0x00. That path was therefore already not a reliable
  end-of-option-list route before this change; ljust changes which non-zero value
  is produced, not whether it terminates. No existing test exercises it. Narrow, but
  worth a look on its own.

Provenance of the measurements

Every figure above was produced with PYTHONSAFEPATH=1, the target tree inserted
at sys.path[0], assert pcapkit.__file__.startswith(<tree>) before any other
import, and the padding side read back out of inspect.getsource(FieldBase.unpack)
and printed alongside the result — so no number here is attributed to a tree it was
not measured on. MemoryError work ran under an explicit RLIMIT_AS (1 GiB or
4 GiB, stated per result) because this host has 62 GB RAM and no swap.

[JarryShaw]

✅ GOOD TO MERGE — head 2a60e14120c119710603b37b4c13477d49df15d7. Independently reproduced both load-bearing claims: the pinned test in test_tcp_udp_unit.py fails with exactly the claimed old-padding-vs-new-ljust mismatch (2 failed, both subtests, nothing else in the file breaks — 17 passed), and the many_interfaces.pcapng-at-5222-octets ValueError→ValueError shift reproduces exactly (393216 is not a valid BlockType under old rjust vs read length must be non-negative or -1 under new ljust), with full parses identical at 1594 frames across all 20 real captures both ways. Also independently reproduced the disclosed MemoryError→ValueError chain fix under a 1 GiB RLIMIT_AS. See appendix.

[JarryShaw]

Cross-review appendix — PR #621

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head 2a60e14120c119710603b37b4c13477d49df15d7 in an isolated worktree, pcapkit.__file__ asserted resolving there before trusting any result (the shared venv's editable install otherwise resolves to a different, stale checkout on this machine).

Methodology note: a bytecode-cache trap, caught mid-review

This repo's worktrees accumulate __pycache__ directories, and toggling field.py between rjust/ljust via sed + git checkout without clearing them produced a false result on my first attempt at the targeted check below — a stale compiled .pyc was silently reused across the toggle, so both states reported the same error. Confirmed by finding 54 leftover __pycache__ dirs in the worktree. Every result below was re-run with find <worktree> -name __pycache__ -type d -exec rm -rf {} + and PYTHONDONTWRITEBYTECODE=1 immediately before each toggle; noting it here since it's a trap anyone else touching this worktree will hit too.

The correctness claim (not re-derived, per brief)

ljust vs rjust for a short read: not re-derived, already established.

Load-bearing item 1 — the deliberately-failing test, confirmed to fail exactly as claimed

tests/protocols/transport/test_tcp_udp_unit.py::test_a_truncated_option_still_parses_its_declared_length, whole-file run:

2 failed, 17 passed, 3 warnings in 28.01s
SUBFAILED(declared_length=12) ...
SUBFAILED(declared_length=32) ...

The failure is exactly the claimed mismatch — expected b'\x00'*zeroes + trailing (old rjust side), got trailing + b'\x00'*zeroes (new ljust side) — at test_tcp_udp_unit.py:1349. Nothing else in the file broke (17 passed). Correctly left untouched since #612 owns that file.

Load-bearing item 2 — truncated-capture-still-parses, independently swept

Built a truncation harness against all 20 real capture files in examples/captures/ (.pcap/.pcapng/.cap — the generic glob misses http6.cap on a first pass, worth flagging for anyone repeating this).

Full parses, both sides identical: 1594 frames total across all 20 files, both under head (ljust) and a locally-reverted rjust (reverted in-place, never committed, restored after each check) — matches the PR's claim exactly, including the file count.

The one claimed divergence, reproduced exactly: truncating many_interfaces.pcapng to precisely 5222 octets:

• OLD (rjust): ValueError: 393216 is not a valid BlockType
• NEW (ljust): ValueError: read length must be non-negative or -1

Both are ValueErrors — loud parse failures, not a silent misparse — satisfying the #571/#431 constraint. A broader 159-point sweep (generic offsets: several fractional cut points plus tail-truncations per file) found zero further differences, consistent with — though not an exact reproduction of — the PR's own "231 of 232 identical" count; my generic offsets simply didn't land on the same narrow divergence window the PR's own sweep found.

Spot-checked: the disclosed MemoryError→ValueError chain (#594)

Truncated dhcp_little_endian.pcapng to 161/641/1389 octets (the PR's own cited offsets) under resource.setrlimit(RLIMIT_AS, 1 GiB):

• OLD (rjust): MemoryError at all three offsets.
• NEW (ljust): ValueError: read length must be non-negative or -1 at all three offsets, no allocation attempted.

Matches the PR body's table exactly.

Subtest-count reconciliation

Ran tests/corekit/test_fields_field.py in isolation (clean pycache): 35 passed, 0 failed, 133 subtests passed. The PR body's own number for the new FieldBaseShortReadPaddingSideTests class alone is "133 subtests" (10 new test methods, 25 pre-existing ones in the same file deselected in their run) — my whole-file run (35 = 10 new + 25 old) reports the same 133 subtests, so this reconciles exactly once I saw the PR's own breakdown; I could not have derived the "195 → 328" figure myself since that spans the whole tests/corekit/ directory, which is out of this review's scope (host-safety: never run a whole subtree beyond what's needed). Flagging as reconciled against the PR's own stated breakdown, not independently re-derived at the wider scope.

Not independently checked

• The tests/protocols/ 1467-subtest / 2-failed claim ("see below" in the PR body) — did not locate or rerun whatever those 2 failures are; out of scope for the two load-bearing claims this review targeted, and running that directory is borderline for this host's no-swap constraint.
• The disclosed-but-out-of-scope SeekableReader.truncate rjust bug at pcapkit/corekit/io.py:328 — read the reasoning, did not reproduce the b'abcd'/truncate(8)/seek(0) repro myself. Correctly out of scope for this PR.
• The partial-2-octet-type-field finding across hip.py/sctp.py/mh.py — read, not independently exercised.
• Coverage-flat claim (84% before/after) — not re-run; the brief explicitly says not to credit this axis, and the subtest axis above is the one that matters.

Disagreement log

None. Every claim I checked — the pinned test's exact failure shape, the full-parse identity across 20 captures, the specific many_interfaces.pcapng/5222-octet divergence, and the disclosed MemoryError→ValueError chain — reproduced exactly as the PR describes.

[JarryShaw]

✅ GOOD TO MERGE — head 2a60e14120c119710603b37b4c13477d49df15d7. Independently reproduced the exact many_interfaces.pcapng/5222-byte divergence the PR discloses (both sides still raise, just a different ValueError), confirmed the one known test failure is exactly and only the two named subtests (owned by the separate PR #612), and confirmed every full-capture parse across 19 available fixtures is byte-identical between old and new code. See appendix.

[JarryShaw]

Cross-review appendix — PR #621

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head 2a60e14120c119710603b37b4c13477d49df15d7 in an isolated worktree. pcapkit.__file__ confirmed resolving inside that worktree (not the shared venv's stale main-checkout editable install) before running anything.

Diff scope

git diff af1f771b9..2a60e1412 -- pcapkit/corekit/fields/field.py is exactly the disclosed change: buffer[:length].rjust(length, b'\x00') → buffer[:length].ljust(length, b'\x00') at the struct.unpack call, plus a rewritten explanatory comment above it. Nothing else in the file changed. (Note: af1f771b9 is not this branch's actual fork point — its real merge-base is a2be2cc1a, same base-drift pattern seen across this whole PR wave — but the file-scoped diff is identical either way since nothing else touches field.py.)

The rjust→ljust correctness argument (left-pad puts the zeros at the front, asserting the unread octets were the leading ones, which is wrong for every byte order since a short read always loses the trailing octets) was independently derivable from the buffer-store semantics and is taken as given per the review brief; not re-derived here.

The known-failing test — confirmed exact

pytest tests/protocols/transport/test_tcp_udp_unit.py -q
→ SUBFAILED(declared_length=12) test_a_truncated_option_still_parses_its_declared_length
→ SUBFAILED(declared_length=32) test_a_truncated_option_still_parses_its_declared_length
→ 2 failed, 17 passed, 3 warnings, exit 1

(Exit code read from the actual pytest process, not through a pipe to tail.) Nothing else in the 19-test file fails. Read lines 1295-1349 of the test: its docstring explicitly says "unpack() left-pads the short read with zero octets" and its assertion is self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing) — pinned to the pre-fix rjust behavior. git diff af1f771b9..4ce6b1382 --stat -- tests/protocols/transport/test_tcp_udp_unit.py (PR #612's current head) shows 112 changed lines in that exact file, confirming PR #612 is the one that updates this pinned assertion. #621 correctly leaves it alone.

The shippability sweep — reproduced, plus one additional case beyond what was disclosed

Built a bounded, single-process sweep (19 real captures in examples/captures/, no synthetic files):

Full parses, untruncated, both ways: all 19 captures parse to identical frame counts under both rjust (temporarily reverted, uncommitted, then restored — git diff confirmed clean afterward) and ljust: TOTAL_FRAMES=1568 both times, every per-file count identical. (The PR's own count is 1594 across ~20 captures; this is a subset — 19 locally available vs. their fuller sweep — consistent with, not a repro discrepancy against, their number.)

Truncation sweep, ~12 cut points each across 5 files (many_interfaces.pcapng, profile.pcapng, test.pcapng, dhcp.pcapng, tcp.pcap), old vs. new:

• The disclosed case, reproduced exactly: many_interfaces.pcapng cut at byte 5222 (the exact offset named in the PR) — old (rjust) raises ValueError: 393216 is not a valid BlockType; new (ljust) raises ValueError: read length must be non-negative or -1. Both are ValueErrors, both still cleanly reject the truncated capture — no crash-to-silent-success flip. This is exactly the corekit: reject short dynamic field buffers #571/OptionField.unpack never returns for a well-formed HOPOPT header with an SMF_DPD option #431 safety property holding.
• A second, previously-undisclosed divergence found in this sweep: test.pcapng cut at byte 2035 — old raises ProtocolError: PCAP-NG: [isb_starttime] invalid length (expected 8, got 2048); new raises ValueError: read length must be non-negative or -1. Different exception class this time (not just message), but again both are exceptions — the safety property still holds, it just wasn't one of the cases the PR body named.
• All other sampled cut points (~58 of 60 data points across the 5 files) gave identical errors on both sides.

Given my ~60-point coarse sweep already turned up 2 divergences (~3%) against the PR's own reported 1-in-232 (~0.4%), the true divergence rate is likely higher than "1 of 232" suggests, or the two sweeps used different (non-comparable) sampling methodologies — I can't tell which without their script. Either way, every divergence found, in both sweeps, preserves the load-bearing property: the padding side changes what a truncated field decodes to, which can flip which exception fires, but truncated input never silently parses in one direction and errors in the other. That property, not the specific count, is what makes this shippable.

field.py restored to the PR's exact content before finishing; worktree left clean and in place (it predates this review).

gh pr view

closingIssuesReferences = [604], confirmed. mergeable: CONFLICTING / mergeStateStatus: DIRTY — same changelog-file rebase collision seen on every open PR in this wave, not a code issue.

Not independently checked

• The author's own claim of exactly "20 captures" and "1594 frames" was not reproduced 1:1 — only the 19 captures present in this worktree were available; no reason found to doubt the fuller original count.
• Did not attempt to enumerate every possible truncation byte offset (computationally large); the ~60-point sweep is a sample, not exhaustive.

Disagreement log

One, minor: the PR's "231 of 232 outcomes byte-identical" framing reads as near-total agreement between old and new; an independent, much smaller sweep found a second divergence not mentioned in the PR body (test.pcapng/cut=2035). This doesn't change the merge recommendation — the safety property holds in every case found — but the PR body's precision on "how rare" divergence is may be an undercount, worth a one-line mention if the author revises the description.

[JarryShaw]

❌ NEEDS CHANGES — head 2a60e14120c119710603b37b4c13477d49df15d7. Dissent from the two ✅ verdicts above, on one fact they predate. Both rest on "correctly left untouched since #612 owns that file" — but #612 merged as e55fe0c59 at 03:03:45Z, before those comments were posted at 03:25/03:26. tests/protocols/transport/test_tcp_udp_unit.py is on main now, so nobody else owns it and the two failing subtests are this PR's to fix. Everything else in those reviews I independently corroborate, and I add a denser sweep that slightly corrects the PR's own "one differing outcome" figure. See appendix.

[JarryShaw]

Cross-review appendix — PR #621

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head 2a60e14120c119710603b37b4c13477d49df15d7 in a dedicated worktree, PYTHONSAFEPATH=1 with PYTHONPATH pinned and pcapkit.__file__ asserted into that worktree before every run (printed: …/.claude/worktrees/pr621-verify/pcapkit/__init__.py). Fixtures regenerated wholesale via examples/generators/make_samples.py (24 files) rather than partially, to avoid a mixed vintage. The rjust↔ljust core derivation was already established and was deliberately not re-derived.

The change itself

pcapkit/corekit/fields/field.py:527 (the line the issue cites as :244; the shift is real — #593's zero-pad budget work landed above it). rjust → ljust, unconditional, no byte-order branch, with a comment recording the four measured figures. A second hunk at :427 is wording-only (rjust() → "the padding", zero-pad → zero-fill) inside an existing comment.

What this PR leaves failing — confirmed exactly, and nothing more

pytest -q tests/protocols/transport/test_tcp_udp_unit.py
→ 2 failed, 17 passed, exit 1
SUBFAILED(declared_length=12) …::test_a_truncated_option_still_parses_its_declared_length
SUBFAILED(declared_length=32) …::test_a_truncated_option_still_parses_its_declared_length

Both subtests of that one test, and no other test in the file. The assertion detail confirms the cause is purely the padding side, not a semantic regression:

E  AssertionError: b'\xaa\xbb\xcc\xdd\xee\xff\x00\x00\x00\x00'   (actual, ljust — tail-padded)
               != b'\x00\x00\x00\x00\xaa\xbb\xcc\xdd\xee\xff'   (expected, pins rjust)

The two files this PR does update are green: tests/corekit/test_fields_field.py + tests/protocols/internet/test_ipv4_unit.py → 61 passed, 164 subtests passed, exit 0.

Two measurement notes for anyone reproducing this. First, PYTEST_EXIT read through a pipe reports the last command's status — the real pytest exit code is 1 here, and pytest 9.1.1 does surface SUBFAILED natively. Second, an earlier run of mine in a different worktree reported a spurious 86 failures; that worktree had field.py concurrently reverted to rjust by another process doing an A/B comparison (git status showed it modified). Always check the worktree is clean before trusting a count — the 86 figure is an artefact and not a property of this PR.

The blocker, and why it changed since this PR was written

The PR correctly declined to edit test_tcp_udp_unit.py on the grounds that #612 owned it. That is no longer true: #612 merged as e55fe0c59, and #613/#614 have landed too, moving main from af1f771b9 to c260f705b. The file is on main now, with the old padding side still pinned in three places:

• :1374 — "left-pads the short read with zero octets rather than raising"
• :1386 — "pinning that the padding scales with the declared length"
• :1415 — self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing)

So the work is unblocked and belongs to this PR: rebase onto current main (the branch forks from a2be2cc1a, and note af1f771b9 is not an ancestor of it — the review brief's stated base is wrong for all three open PRs in this wave), then flip :1415 to the tail-padded expectation and correct the prose at :1374/:1386. Until that lands, merging this turns main red.

The #571/#431 constraint — independently swept, and it holds

Built my own sweep rather than trusting the reported one: for each of 20 real captures, parse fully and record frame count plus a digest of every frame's protocol chain, then re-parse at 13 truncation points per file (fractional offsets plus fixed small offsets and the 5222 the PR names). Ran it twice — once on this head, once with only line 527 reverted to rjust — and diffed the JSON.

files swept:                        20
total frames across full parses:    1594
full-parse outcomes differing:      0 of 20      <-- the constraint
truncation points:                  260
truncated outcomes differing:       3
ljust: 2 truncations parsed, 258 raised
rjust: 2 truncations parsed, 258 raised

Zero full parses change outcome, and the 1594-frame total matches the PR's figure exactly. Decisively, the parsed-vs-raised split across truncations is identical on both sides (2 / 258) — there is no truncation that used to raise and now silently yields wrong data, nor the reverse. That is the property that would have made this unshippable, and it is clean.

One correction to the PR's claim

The PR reports "231 of 232 outcomes byte-identical, the single difference being many_interfaces.pcapng at 5222 octets". I reproduced that named case exactly:

many_interfaces.pcapng @ 5222
  ljust: ValueError: read length must be non-negative or -1
  rjust: ValueError: 393216 is not a valid BlockType

But on a denser grid (260 points rather than 232) three differ, not one — the two extra points simply weren't sampled by the PR's grid:

profile.pcapng @ 150
  ljust: ValueError: read length must be non-negative or -1
  rjust: ValueError: 65536 is not a valid BlockType

test.pcapng @ 1001
  ljust: ProtocolError: PCAP-NG: [epb_packetid] invalid length (expected 4, got 0)
  rjust: ValueError: read length must be non-negative or -1

All three remain exception→exception, so none weakens the conclusion. Worth noting though that test.pcapng @ 1001 changes the exception type, not merely the message (ValueError → ProtocolError) — a slightly broader behavioural delta than "one differing outcome" conveys, and arguably an improvement, since a typed ProtocolError beats a bare ValueError. I'd suggest the PR text say "three of 260 on a denser grid, all exception-to-exception, one of them changing exception type" rather than "one of 232".

Coverage

Per the brief, the flat 84% is correctly not treated as evidence — line 527 already executed before the change. The meaningful axis is the subtest count, and the 164 subtests passing in the two updated files is consistent with the claimed 195 → 328 growth across the PR's full selection, which I did not run in full (host-safety: no whole-tree runs).

Disagreement log

The fix: agree. Correct, minimal, well-commented, and the constraint that mattered is verified intact by an independent sweep.

Verdict is NEEDS CHANGES on one narrow point only: the PR leaves test_tcp_udp_unit.py red, and the justification for that has expired now that #612 has merged. This is mechanical — three lines in one file — not a rethink.

One dispute with the PR's own numbers: "231 of 232, one difference" understates it slightly; a denser grid gives three of 260, one of which changes exception type. Conclusion unaffected.

One dispute with the review brief rather than the PR: af1f771b9 is not an ancestor of this branch (true merge-base a2be2cc1a), so diffs taken from it misattribute unrelated merged work to this PR.

[JarryShaw]

Retraction: I am withdrawing my ✅ GOOD TO MERGE above and joining the ❌ NEEDS CHANGES verdict.

My verdict rested on "PR #612 owns test_tcp_udp_unit.py, so #621 correctly leaves it alone." That premise is now confirmed wrong on two counts, independent of the merge-timing issue the dissenting review raises:

1. test(tcp): reach the MP_JOIN dispatchers through TCP(), not a hand-written _flags (#603) #612's 112 lines of changes to that file never touched this test. I checked git diff af1f771b9..4ce6b1382 --stat -- tests/protocols/transport/test_tcp_udp_unit.py and saw 112 changed lines, and wrongly inferred those changes covered test_a_truncated_option_still_parses_its_declared_length. They don't — test(tcp): reach the MP_JOIN dispatchers through TCP(), not a hand-written _flags (#603) #612's changes were about the MP_JOIN dispatcher/cast('Enum_Flags', 0) work, an unrelated part of the same file.
2. Confirmed directly against current origin/main (which already includes merged test(tcp): reach the MP_JOIN dispatchers through TCP(), not a hand-written _flags (#603) #612/docs: repair the Code of Conduct's rendering and refresh CONTRIBUTING #613/fix(pcap): seed the frame byte order under the key the reader consults (#605) #614/docs: add CITATION.cff and bring the copyright notice up to date #615): git show origin/main:tests/protocols/transport/test_tcp_udp_unit.py still pins the pre-fix rjust behavior at the assertion self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing), with the docstring above it still saying "left-pads the short read with zero octets." Nobody's merged PR has updated it.

So the dissenting review's read is correct: this file is unowned, still red under #621's fix, and merging #621 as-is turns main's test suite red. My apologies for the incorrect premise in my original review — the rest of that review (diff scope, the shippability sweep, the many_interfaces.pcapng/5222 reproduction) stands, but the merge recommendation does not. ❌ NEEDS CHANGES, in agreement with the review above: rebase onto current main and fix the three pinned sites in test_tcp_udp_unit.py (~:1374, :1386, :1415 on current main) before this merges.

[JarryShaw]

This merged before the NEEDS CHANGES verdict and my retraction above were posted, and main is now failing a test as a result.

Merge timestamp 2026-09-22T03:28:24Z (squash commit dfc23b8b8), the NEEDS CHANGES verdict landed at 03:30:20Z, my retraction at 03:32:18Z — the merge preceded both by 2-4 minutes.

Verified directly against current origin/main (tip dfc23b8b8, includes this squash-merge):

PYTHONSAFEPATH=1 PYTHONPATH=<worktree> python -m pytest tests/protocols/transport/test_tcp_udp_unit.py -q
→ SUBFAILED(declared_length=12) test_a_truncated_option_still_parses_its_declared_length
→ SUBFAILED(declared_length=32) test_a_truncated_option_still_parses_its_declared_length
→ 2 failed, 17 passed, exit 1

The squash-merged commit's tests/protocols/transport/test_tcp_udp_unit.py is unchanged from this PR's own branch — self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing) still pins the pre-fix left-pad behavior, and the docstring above it still says "left-pads the short read with zero octets." Nothing in this PR or the three PRs merged just before it (#612, #613, #614) touched that assertion.

main is red right now on this one test. The fix is mechanical — flip the assertion to trailing + b'\x00' * zeroes and correct the two lines of docstring prose describing the old padding side (around what is now :1374/:1386/:1415 in that file) — but it needs a follow-up commit; this comment is a heads-up, not an attempt to push one.

[JarryShaw]

Cross-review — PR #621 (posted after merge; main is currently red because of this change)

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head 2a60e14120c119710603b37b4c13477d49df15d7. This PR merged at head fe4ed4242db36d9146d6e60f09f4f054712c8753 (merge commit dfc23b8b807f950d0e48fd7e9da5c6e3c5d50640, 2026-09-22T03:28:24Z) while the review was running.

The fix itself is correct and is exactly what shipped — pcapkit/corekit/fields/field.py is blob 81ee440b2dc4e42ef055300c0c02eb8445f92e83 at the head I reviewed, at the head that merged, and on origin/main right now. But the deferred test did not get resolved by #612 landing, and main is failing as a result. That is the headline; everything else is below it.

main is red, and this change is the proximate cause

Verified in a clean throwaway worktree at origin/main (c43ca8c66), pcapkit.__file__ asserted to resolve into it, Python 3.14.7:

$ pytest tests/protocols/transport/test_tcp_udp_unit.py -q
SUBFAILED(declared_length=12) ...::test_a_truncated_option_still_parses_its_declared_length
SUBFAILED(declared_length=32) ...::test_a_truncated_option_still_parses_its_declared_length
2 failed, 17 passed

Causation established by reverting only the one changed line on that same clean tree:

$ sed -i 's/buffer[:length].ljust(length/buffer[:length].rjust(length/' pcapkit/corekit/fields/field.py
$ pytest tests/protocols/transport/test_tcp_udp_unit.py -q
17 passed, 2 subtests passed

So the failure is this change's, not an unrelated breakage or a fixture problem.

Why the handoff did not work. This PR deliberately left that test alone on the stated grounds that #612 owned the file. #612 did rewrite it — the blob moved from 1efa219d7 to ad5229d64 — but it kept the old padding-side expectation, in two places:

• tests/protocols/transport/test_tcp_udp_unit.py:1415 — self.assertEqual(unassigned.data, b'\x00' * zeroes + trailing), which pins zeros first, i.e. the rjust layout.
• The docstring around line 1374 still says unpack "left-pads the short read with zero octets" and describes "a data value of four zero octets followed by the six real ones."

Both need to flip to trailing-pad. The assertion becomes trailing + b'\x00' * zeroes, and the prose should say right-pads. Neither PR was wrong to think the other would handle it; the file simply changed hands without the assertion changing with it.

What I did verify about the fix, independently

• Diff scope. af1f771b9 is not this PR's merge-base (a2be2cc1a is), so diffing against it spuriously shows SECURITY.md churn from docs: say in SECURITY.md that hostile captures are a live risk, not a closed one #611. Against the real merge-base the PR touches only the two changelogs, field.py, tests/corekit/test_fields_field.py, and tests/protocols/internet/test_ipv4_unit.py. The code change is the rjust→ljust swap plus comments.
• No intact capture changes behavior. All 20 real captures in examples/captures/ parse to identical frame counts and identical decoded-content hashes under both paddings — 1594 frames, 0/20 differences.
• Blast radius on truncated input is crash-to-crash only. 428 truncation points across 8 captures: 421 identical, 2 differences, both on many_interfaces.pcapng (off=133 ValueError→ProtocolError; off=141 ValueError→struct.error). The PR's own cited case reproduced exactly at offset 5222: old ValueError: 393216 is not a valid BlockType → new ValueError: read length must be non-negative or -1. Three more of the same shape at 5223, 5241, 5242. Zero cases of a raise becoming a silently-returned wrong value, in either direction — which is the property that makes this safe to ship at all.
• many_interfaces.pcapng is the genuine fixture, not the offline stand-in: examples/generators/pcapng.py fetches it from the Wireshark tree with a recorded digest, and make_samples.py reported [downloaded (already present)] ... 20888 bytes, unchanged; 64 frames.
• The subtest delta holds. -k FieldBaseShortReadPaddingSideTests at the reviewed head: 10 passed, 133 subtests passed. Reverted to rjust: 85 failed, 8 passed, 50 subtests passed. Both exactly as the PR reports.

Disagreement log

The PR's tests/corekit/ row is wrong, confirmed by two independent measurements. The body reports 144 passed, 328 subtests passed (and a 195 → 328 baseline). Measured at the reviewed head, with test_fields_field.py confirmed byte-identical (blob dde1b697):

159 passed, 5 warnings, 372 subtests passed in 129.83s

That is +15 tests and +44 subtests above the reported figure, and the discrepancy is the same 44 on both sides of the delta — so the +133 delta the PR draws from it is right even though both absolute numbers are off. Most likely the whole-directory row was transcribed from a different environment or conflated with the single-class row, which happens to also be 328-adjacent. Not a correctness issue, but the table should not be quoted as a baseline by anything downstream.

Two corrections to the review process, for the record

• An earlier pass of this review reported git ls-remote origin main as 8cfd6ab01, and inferred that the merge had not reached the branch. That was a misread of a stale local main 25 commits behind; the real remote ref was already well past it and is c43ca8c66 now. There is no ref inconsistency — the merge landed normally.
• The worktree used for the early measurements was not exclusive: another process was operating on the same path, it has since been rebased to 3885dad39 (a third, non-shipped variant of the test file, blob 7034d5475), and one edit to the shared field.py was made before the collision was noticed. Every number quoted above was therefore re-established either in an isolated copy or, for the main-red finding and the tests/corekit/ count, by me directly against a pinned blob.

Not independently checked

• pcapkit/corekit/io.py:328's own rjust defect in SeekableReader.truncate, which this PR flags and defers.
• The 2-octet option-type partial-read edge case it also lists as found-but-unfixed.
• The PR's exact "232 outcomes, 231 identical" aggregate — the pattern and its one cited case reproduced, but on different sample points.
• Coverage percentage, deliberately: it is flat at 84% because line 506 already executed, so it carries no signal here.

Recommended follow-up

A one-line change to tests/protocols/transport/test_tcp_udp_unit.py:1415 plus its docstring, flipping the expectation to trailing-pad, would make main green. I have not opened an issue or a PR for it — flagging it here for the owner to route.