test: pin the truncated-data half of the #431 short-read accommodation

[JarryShaw]

Summary

Closes the coverage gap tracked by #572: only the empty-tail half of the
#431 accommodation had a test (test_an_option_area_longer_than_the_{segment,datagram}_still_parses
in the TCP/IPv4 unit files). The truncated-data half — an option that
starts, declares more data than the capture actually holds, and runs out
partway through its own data field — had none, and that gap is what let
a candidate fix for #554 (#571) regress it while the whole suite stayed
green. This is a test-only change; no source file is touched.

• TCP (tests/protocols/transport/test_tcp_udp_unit.py): a segment
  with data offset 7 (8 octets of option area) carrying a reserved
  option kind (0x4f, Option.Reserved_79) that declares length=12
  (and, in a second subtest, length=32) with only 6 real data octets
  behind it. On main this parses: FieldBase.unpack left-pads the short
  read with zero octets rather than raising, so the option's declared
  length survives intact and its data comes back as the zero-padded
  reconstruction. Reachable here because TCP._read_tcp_options sizes
  each parsed option by len(schema) — what it actually consumed — not
  by its self-reported length, so its own threshold check never sees
  the shortfall.
• IPv4 (tests/protocols/internet/test_ipv4_unit.py): the same
  UnassignedOption.data field, same shortfall, same left-padding
  outcome — but IPv4._read_ipv4_options sums each option's
  self-declared length rather than what it consumed, so the option area
  has to be declared with headroom above the single option's declared
  length, or that check discards the accommodation's result once its loop
  over the parsed options finishes. The docstring on the test spells out
  the resulting second-order effect (Schema.unpack's pre-existing
  option_padding rewind, from Fix seven PCAP-NG parser defects (#341-#347) #371, re-reading the same octets once
  more as padding) and why it's immaterial to what the test actually pins.

Both tests were proven to fail against a deliberately over-strict guard —
FieldBase.unpack raising whenever len(buffer) < length — with the same
shape of FieldValueError #571 would produce, then verified to pass again
once the guard was reverted (byte-identical restore confirmed by
md5sum).

A changelog entry was added to docs/source/changelog/1.5.0.rst and
CHANGELOG.md regenerated with util/changelog_md.py.

On #571

The reason this gap exists at all is lux-liang's #571, which regressed
this exact path while the full suite passed — that PR is the occasion for
this test, and is cited as such in both files. #571 already covers the
sibling fixed-width short-read case in its own
test_unpack_preserves_fixed_length_short_input_compatibility; this PR
is independently written and covers only the callable-length case
(BytesField(length=lambda pkt: ...)) exercised through a full TCP/IPv4
parse, which is the half #571 does not touch.

Test plan

• pytest tests/protocols/transport/test_tcp_udp_unit.py tests/protocols/internet/test_ipv4_unit.py -k test_a_truncated_option_still_parses_its_declared_length -q — 2 passed, 2 subtests passed
• Same tests run against a deliberately over-strict FieldBase.unpack guard — 3 failed (2 subtests + 1 IPv4): FieldValueError: Field data requires 10 octets, but only 6 are available. for the length=12 cases, and requires 30 octets, but only 6 are available for the length=32 subtest
• Guard reverted, byte-identical to original confirmed via md5sum, tests pass again
• Full unit tier: pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py' — 1118 passed, 8 skipped, 2670 subtests passed
• python util/changelog_md.py --check clean

---

Edit: this PR merged before its cross-review (on a different model,
per this repo's practice) finished. The review found the tests themselves
— every constructed byte, assertion, and pass/fail outcome — correct, but
found several factual errors in the docstring prose above and in the
tests' own docstrings, corrected in #580: this description's text has been
updated to match. See #580 for the full list and reasoning.

[JarryShaw]

✅ GOOD TO MERGE — at 0a1e66bd9.

Cross-model review (Opus 5; PR authored on Sonnet). Test-only, 4 files, +136/−0, no source file touched.

I had built this case from #572's text before reading the PR, so the comparison is genuinely independent: on main the segment parses to options=[('79', 12)] with data=b'\x00\x00\x00\x00\xaa\xbb\xcc\xdd\xee\xff'. The test asserts the zero-padded value, which is the easy thing to get wrong here — the six real octets arrive left-padded to the declared width, not bare.

What I derived rather than accepted:

• The TCP/IPv4 asymmetry the docstrings turn on is real, and correctly diagnosed. TCP._read_tcp_options bills counter += len(schema) — what was consumed (tcp.py:698); IPv4._read_ipv4_options bills counter += data.length — what was declared (ipv4.py:608, raising at :616-617). That is exactly why IPv4 needs ihl headroom and TCP does not.
• The phantom-EOOL arithmetic checks out. IPv4 repacks to 36 octets = 20 + the full 16-octet declared option area, matching the docstring's account of the same 8 octets being handed to the schema a second time as padding. bytes(proto.__header__) == raw is True for TCP (28 vs 28) and False for IPv4 (36 vs 28) — so asserting it in one file and explicitly declining to in the other is right, and the PR says so rather than quietly omitting it.
• I proved the failure myself, replacing the guard with if len(buffer) < length: (the corekit: reject short dynamic field buffers #571 shape): exactly 3 failures — 2 TCP subtests + 1 IPv4 — each FieldValueError: Field data declares a length of 10 octet(s), but only 6 octet(s) are available. Restored; git diff empty; re-ran green.
• The check this PR could not have run itself. It is 6 commits behind main: its base is 8cfd6ab01, and main is now 1c5833e00 — which includes fix(corekit): bound FieldBase.unpack's rjust() padding to a sane ceiling (#554) #569, the one PR that changes FieldBase.unpack. I merged main in locally (clean, no conflicts) and re-ran: 15 passed / 2 subtests / exit 0 across the truncation, empty-tail and FieldBaseUnpackBounds tests together, and 43 passed / 33 subtests / exit 0 for both touched files in full. The two changes are compatible. Temp branch deleted after.
• util/changelog_md.py --check in step. Issue The #431 truncated-option short read has no test, only its empty-tail case does #572's own test_ipv4_unit.py:1977 citation is accurate.

Two non-blocking notes and one unverified claim in the detailed comment.

[JarryShaw]

Detailed cross-review — #578 @ 0a1e66bd9

Reviewer: Opus 5, per the standing rule that an agent-raised PR gets a cross-review from a different model than the one that wrote it. All runs with PYTHONSAFEPATH=1 and PYTHONPATH pinned to my worktree, asserting pcapkit.__file__ resolves inside it before importing anything else. Exit codes read from files, never from a summary line — which turned out to matter here, see the note at the end.

Why my baseline is genuinely independent

I constructed the truncated-option case from #572's prose before this PR existed, as groundwork for whatever fix landed. On main (8cfd6ab01) it gave:

declared=12 -> parses, options=[('79', 12)]
               data=b'\x00\x00\x00\x00\x01\x02\x03\x04\x05\x06'
declared=32 -> parses, options=[('79', 32)]
               data=b'\x00'*24 + b'\x01\x02\x03\x04\x05\x06'

So the two things I most wanted to see in a test for this — that the declared length survives, and that data is the left-zero-padded reconstruction rather than the six octets actually present — are both what this PR asserts. That padding is the subtle half. A test written from the issue text alone could easily have asserted data == trailing and failed for the right-sounding wrong reason.

Using bytes([0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff]) for the real octets is a better choice than sequential small integers, because it makes the zero padding unmistakable in a failure diff rather than something you have to count.

The asymmetry between the two files is real

Both docstrings rest on a claim about why TCP reaches the accommodation with an exactly-sized option area while IPv4 needs headroom. I checked the two accounting loops directly rather than taking it:

pcapkit/protocols/transport/tcp.py:698      counter += len(schema)        # consumed
pcapkit/protocols/transport/tcp.py:705-706  if counter > size:  raise ProtocolError('TCP: invalid format')

pcapkit/protocols/internet/ipv4.py:608      counter += data.length        # self-declared
pcapkit/protocols/internet/ipv4.py:616-617  if counter > length: raise ProtocolError('IPv4: invalid format')

That is the whole difference, and it is stated correctly. TCP's single option consumed 8 of 8, so 8 > 8 is false and nothing raises. IPv4 would bill the option's declared 12 against a declared 8 and raise IPv4: invalid format before the field-level accommodation was ever reached — hence ihl=9. The docstring's explanation of why declaring exactly 8 would not work is not hand-waving; it names the right mechanism.

The second-order effect, and the honesty of not asserting it

The IPv4 docstring says the 16-octet budget outliving the 8 octets consumed makes the loop read a phantom exhausted option, decode it as end-of-option-list, and causes the #431 machinery to hand the same 8 octets back as padding — so bytes(proto.__header__) does not round-trip, and it declines to assert it. Measured:

TCP  declared=12  hdr_len=28  bytes(__header__) == raw ? True    (28 vs 28)
TCP  declared=32  hdr_len=28  bytes(__header__) == raw ? True    (28 vs 28)
IPv4             hdr_len=36  bytes(__header__) == raw ? False   (36 vs 28)
ihl declares 16 octets of options; raw carries 8

36 = 20 + 16 — the full declared option area, i.e. the 8 real octets plus 8 more. That is precisely what the docstring predicts, arithmetically. I went in expecting to find an overclaim here, since a schema holding a 10-octet data field ought to repack to more than the 8 octets it came from, and instead found the account correct and the TCP round-trip genuinely holding. Asserting it where it holds and explaining where it does not is the right treatment, and it is the sort of thing that usually gets quietly dropped instead.

Other constants re-derived independently:

Option.get(0x4f)     = <Option.Reserved_79: 79>
OptionNumber.get(31) = <OptionNumber.Unassigned_31: 31>
IPv4 options         = [(Unassigned_31, 12), (EOOL, 1)]
IPv4 data            = b'\x00'*4 + trailing        -> True

All as asserted.

I proved the tests fail without the thing they pin

A test-only PR's whole value is its sensitivity, so this is the claim that mattered most. I replaced the real guard with the unconditional form #571 proposed:

# pcapkit/corekit/fields/field.py:285
- if length > _MAX_ZERO_PAD_LENGTH and len(buffer) < length:
+ if len(buffer) < length:            # REVIEW PROBE: over-strict, the #571 shape

Result — exit code 1 read from a file:

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
FAILED tests/protocols/internet/test_ipv4_unit.py::…::test_a_truncated_option_still_parses_its_declared_length
E  FieldValueError: Field data declares a length of 10 octet(s), but only 6 octet(s) are available.
3 failed, 1 passed

Three failures, two of them subtests, one IPv4 — matching the body's claim exactly, and the "10 octets wanted, 6 available" figures match too. (The message wording differs from the body's because the body quotes #571's phrasing and my probe went through #569's, which is now on main; the shortfall is identical.) Guard restored, git diff empty, tests green again.

The check the PR itself could not have made

This branch is 6 commits behind main. Its merge base is 8cfd6ab01; main is now 1c5833e00, having taken #561, #562, #565, #568, #569 and #570 this afternoon. #569 is the one PR in that set that modifies FieldBase.unpack — the exact function this PR's tests exercise. So every run in the test plan predates the change most likely to interact with it, through no fault of the author.

I merged origin/main into a local throwaway branch (clean, no conflicts) and re-ran:

run on #578 + main	exit	result
truncation + empty-tail + FieldBaseUnpackBounds together	0	15 passed, 2 subtests
test_tcp_udp_unit.py and test_ipv4_unit.py in full	0	43 passed, 33 subtests

So the new tests and #569's ceiling coexist, which is unsurprising once you see why — the guard's length > 262144 conjunct means a 10- or 30-octet field never reaches it — but worth having measured rather than reasoned about. The throwaway branch has been deleted (git branch -D review-578-on-main, was 77eeba048); nothing was pushed.

A rebase before merge would be tidy but is not necessary on the evidence above.

Two notes, neither blocking

1. pytest 9.1.1 reported 1 passed alongside those three failures. The TCP parent test shows as PASSED while both its subtests SUBFAILED, because pytest-subtests is absent. This PR uses subTest, so anyone re-running the falsification check must take the exit code from $? and not the summary line — a casual reader of 3 failed, 1 passed could conclude the TCP test was insensitive to the guard when it is the opposite. Not a defect in the PR; a hazard in verifying it, and the reason I read every exit code from a file.

2. The parses emit warnings the tests tolerate silently. SchemaWarning: packet length < 0: -4 and -24 on the TCP cases, and several on the IPv4 one. That is the pre-existing consequence of a header declaring more than it carries and is not introduced here, but nothing in either test acknowledges it. A change that silenced or reworded that warning would pass both tests unnoticed. An assertWarns, or even a docstring sentence naming it as expected, would close that; I would not hold the PR for it.

What I did not verify

1. The body's full unit tier tally (1118 passed, 8 skipped, 2670 subtests). Not reproduced — and it is now unreproducible as stated, since it was measured against the old base and main has since gained test files of its own. I ran both touched files in full on the merged tree instead, which is the part that could plausibly interact.
2. The scope claim about corekit: reject short dynamic field buffers #571 — that it already covers the fixed-width short-read case in test_unpack_preserves_fixed_length_short_input_compatibility, and that this PR covers only the callable-length case. I have standing instructions to leave corekit: reject short dynamic field buffers #571 alone and did not open it, so I take that on trust. It does not affect whether this PR's tests are correct or sensitive, both of which I checked directly.
3. CI, not waited on, per standing practice.

Verdict

GOOD TO MERGE at 0a1e66bd9. It closes exactly the gap #572 describes, asserts the padded reconstruction rather than the naive value, diagnoses the TCP/IPv4 accounting asymmetry correctly at the source, is candid about the one assertion it cannot make and why, and is demonstrably sensitive to the regression it exists to catch — proven by my own probe rather than by its checklist. It also passes on top of the current main that its own runs could not have covered.