Skip to content

fix(generators): correct stale options.py comments and the code they justified (#1337) - #1427

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1337-options-generator-comments
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1337-options-generator-comments

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1337. Each comment was re-checked with a probe on a8cc02594.

  • ILNP nonce (:561): nonce=0 is one octet, and widths 1–16 all build and round-trip. Comment now says so.
  • _httpv2_rebuild: since fix(httpv2): HTTP/2 frames lose octets on rebuild (#1242-#1248, #1251) #1341 make ORs flags in, so the generator now passes frame.flags.__value__ and reserved, like HTTP._make_data. Before, an undefined flag bit (DATA 0x02/0x80, PING 0x40, WINDOW_UPDATE 0x02) or the reserved bit was lost on rebuild.
  • SKIP: _make_tcp_options keeps EOOL/NOP since fix(tcp): from_data reorders and re-pads TCP options #1163, so both now have cases (headers end 00000000 / 01010100, both OK). The rationale now covers only HOPOPT/IPv6-Opts PadN, which list input still collapses onto Pad1.
  • IPv4 extract comment: narrowed to a fresh option list.

Captures: only options-tcp.pcap changes, 27 → 29 frames (2158 → 2306 bytes), the two new TCP padding cases. All others are md5-identical.

Tests: new tests/protocols/test_option_generator_1337_unit.py, 9 subtests fail without the fix. Every module importing the generator: 465 passed, 9 skipped, 5186 subtests, 0 failed.

…justified (#1337)

- ILNP nonce: the width is the ceiling of the bit length in octets, at
  least one; nonce=0 is one octet and every width 1-16 round-trips.
- _httpv2_rebuild: pass the raw flags octet and the reserved bit on, as
  HTTP._make_data does. Since #1341 make ORs flags in, so withholding it
  dropped undefined flag bits and the reserved bit on rebuild.
- SKIP: TCP EOOL and NOP are no longer dropped by _make_tcp_options
  (#1163), so they get cases. The rationale now covers only HOPOPT and
  IPv6-Opts PadN, which list input still collapses onto Pad1.
- IPv4 extract comment: the padding drop applies to a fresh option list.

New tests/protocols/test_option_generator_1337_unit.py; 9 subtests fail
without the fix.
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) 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

Verdict: GOOD TO GO at 2e84ed2a6 (Sonnet cross-review; the author ran on Opus).

  • Tests bite, re-derived by me: the new test_option_generator_1337_unit passes 3/3 on this head. With only options.py put back to main, 9 subtests fail.
  • HTTP/2 rebuild: the reviewer built 216 synthetic frames (types 0–11, flags 0/1/2/4/8/0x20/0x40/0x80/0xFF, reserved 0/1).
    • On main, undefined flag bits and the reserved bit are dropped; DATA 0x02 rebuilds as 0x00.
    • On this head there are 0 byte mismatches among the frames that parse.
    • The flags OR is idempotent for defined bits.
    • Unknown types (0x20, 0xFE) carry flags.__value__.
  • TCP EOOL/NOP: now generated, with distinct headers (00000000 / 01010100) that round-trip OK. HOPOPT/IPv6-Opts PadN correctly stays skipped. Cases go 266 → 268, and no test pins the count.
  • Captures: only options-tcp.pcap changes (27 → 29 frames, 2158 → 2306 bytes). The other four options-* md5s match main. examples/captures/* is gitignored, so nothing needs committing.
  • Comments: the ILNP nonce (1–16 octets round-trip; 17 also works, so the wording is conservative but true) and the IPv4 extract comment were re-probed true.
  • Scope: all three docs(generators): two false claims in options.py — ILNP nonce widths, and the SKIP padding rationale #1337 items are addressed, and only options.py plus the new test change.
  • Neighbours: 16 generator/option/dispatch modules pass, including test_option_roundtrip_unit (364 subtests) and test_capture_tracking.
  • test(generators): the 5-second per-case DEADLINE in options.py fails CI on a stalled runner #1428 (the 5 s DEADLINE): unaffected; the two new TCP cases are cheap.

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

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.46% (unit tier, Python 3.14, 2e84ed2a6, 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 2038 89 634 25 94.76%
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 16615 220 4474 184 98.05%
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 49f855d into main Oct 8, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1337-options-generator-comments branch October 8, 2026 22:23
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

docs(generators): two false claims in options.py — ILNP nonce widths, and the SKIP padding rationale

1 participant