Skip to content

fix(ipv4)!: TS and SEC options do not rebuild byte for byte (#1391, #1392, #1393) - #1399

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1391-1393-ipv4-ts-sec-roundtrip
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1391-1393-ipv4-ts-sec-roundtrip

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — mypy clean on the 3 source files, isort clean on the new test; pylint not run
  • make test passes, and a test case covers the change — scoped: leg protocols/internet, test_option_roundtrip_unit.py, tests/project
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1391
Closes #1392
Closes #1393

Breaking: data TSOption gains a required remainder: bytes, and data SECOption gains a required termination: tuple[bool, ...].

Before → after, from from_data: 44080900 000003e9 → …3e8 / exact; 440b0900 00000005 aabbcc → …000000 / exact; 8204ab91 → 8204ab90 / exact.

Tests: new test_ipv4_ts_sec_roundtrip_unit.py has 5 tests and 18 subtests. With the 3 source files from origin/main, every test fails. With one fix undone at a time, only that issue's cases fail (11 for #1391, 4 for #1392, 3 for #1393). test_ipv4_unit.py adds the new fields in 4 direct constructions. Results: leg protocols/internet 557 tests, 0 failures; round-trip module 6 passed / 362 subtests; tests/project 399 passed, 1 skipped.

…1392, #1393)

- TS: convert a timedelta to milliseconds with `td // timedelta(milliseconds=1)`
  instead of flooring the inexact `total_seconds() * 1000`, which lost 1 ms
  on ~1% of stamps (all four make() paths).
- TS: the 1-3 octets after the last whole slot are now the `remainder`
  BytesField, kept as read, and a new `remainder` data field carried by
  make(); it was a zero-packing PaddingField.
- SEC: the per-octet field termination indicators are now the new
  `termination` data field and are written back on rebuild when they cover
  every octet; otherwise they are set from position as before. The
  non-conforming-indicator warning stays.

Closes #1391
Closes #1392
Closes #1393
@JarryShaw JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Oct 8, 2026
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 8, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on d04716c37: GOOD TO GO (Sonnet reviewed; author Opus).

  • TS fuzz. Of 6,000 random options, the PR rebuilds 4,192 byte-exactly. On main, 2,218 of those mismatched (zeroed tail, 1 ms floor). Both trees reject the same 1,808 inputs at parse, so the PR adds no new raises. The 39 make() cases behave the same on both trees, and boundaries stay exact up to 2³²−1 ms.
  • SEC fuzz. Of 6,000 random options, the PR rebuilds all 6,000 exactly. On main, 5,678 mismatched. A termination tuple of the wrong length falls back to positional bits and never raises.
  • breaking is justified. Both data constructors gain a required field. Across 1,614 fixture frames, the only info difference from main is remainder=b'' on the two TS frames of options-ipv4.pcap.
  • Tests and revert proof. Reverting the three source files fails the new module. protocols/internet passes 557/0. I re-ran the new module myself and got 5/0.

Follow-ups:

@JarryShaw JarryShaw removed the review: running A cross-review is in flight against the current head - no verdict yet label Oct 8, 2026
@JarryShaw JarryShaw added the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.43% (unit tier, Python 3.14, d04716c37, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18791 968 2352 869 90.90%
pcapkit/corekit 2018 88 628 24 94.78%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2637 98 928 48 94.78%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16336 205 4328 167 98.16%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 3653443 into main Oct 8, 2026
42 checks passed
@JarryShaw
JarryShaw deleted the fix/1391-1393-ipv4-ts-sec-roundtrip branch October 8, 2026 03:35
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

1 participant