Skip to content

fix(mh)!: keep the reserved octets MH PaddingField sites capture (#1437) - #1442

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1437-mh-reserved-padding
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1437-mh-reserved-padding

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style — mypy on the 3 MH modules: no new errors vs main (311 both); pylint adds only unused-argument on the new TYPE_CHECKING stub parameters (as every existing stub does); isort not run
  • make test passes, and a test case covers the change — ordering leg protocols/internet 588 OK; test_bitfield_coverage_unit 3 passed; tests/project 419 passed, 1 skipped
  • Changelog entry — N/A — 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?

  • fix — corrects a defect

Description of your pull request and other information

Refs #1437 (part A: MH). Part B (IPv4, L2TP, PCAP-NG) is a separate PR.

Every MH reserved PaddingField that #1436 did not thread, all 38, zero-filled on rebuild. Measured on main: Home Test Init …0101000000aabb… became …010100000000…, and GRE Key 2102aabb became 21020000. The 38 are 8 message bodies, 22 options, 3 Flow Identification sub-options, 1 ANI sub-option and 4 QoS attributes. Each reader now keeps the octets (msg_reserved on a message, apart from the header's reserved; reserved elsewhere). Each maker takes them as a keyword and writes them back through a tolerant getattr. A fresh build still writes zeros.

Breaking: those 38 data models gain a required bytes field. test_mh_unit.py had five hand-built fixtures, which now carry it.

New tests/protocols/internet/test_mh_reserved_padding_unit.py covers parse→rebuild byte-exact and fresh-build zeros for all 38 sites, and checks that the site list matches the schema. On main it fails 40 (38 sub-tests + 2). After make_samples.py, options-internet.pcap info gains only zero-valued msg_reserved/reserved (8 + 22 frames), with no other change.

- 38 MH reserved PaddingField sites (8 message bodies, 22 options,
  3 Flow Identification sub-options, 1 ANI sub-option, 4 QoS
  attributes) were dropped by their readers, so a
  rebuild wrote zeros. The readers now keep them on the data model --
  `msg_reserved` on a message, `reserved` elsewhere -- and the makers
  write them back, with a zero default for fresh builds.
- Breaking: those data models gain a required `bytes` field.
- tests/protocols/internet/test_mh_unit.py: hand-built schemas and data
  models carry the new field.

Refs #1437 (part A: MH)
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) 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

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

  • Completeness. Introspecting the schema finds 40 PaddingField sites in schema/internet/mh.py. Taking away fix(corekit)!: non-zero padding octets are zeroed on rebuild (#1223) #1436's PadOption and PadFlowIdentificationSuboption leaves 38, which match the PR's list by name.
  • Byte-exactness. All 38 cases were rewritten with different octets, and every one rebuilds exactly through both info and info.to_dict(). Fresh builds write zeros, and wrong-width keywords follow fix(corekit)!: non-zero padding octets are zeroed on rebuild (#1223) #1436's PaddingField.pack.
  • The cases are real tests. Each asserts its raw hex differs from the zero form, so none can pass vacuously.
  • ! / breaking is justified. The 38 data models gain a required field. Across the captures only options-internet.pcap changes: 30 of 148 frames gain a zero-valued key, and nothing else differs.
  • Tests, which I re-ran: the new module gives 5 passed with 76 subtests. With the three mh.py files from main it gives 40 failed, 3 passed.
  • Merge. It merges into 7c6203892 with no conflicts. The internet leg is 588 OK, test_mh_unit 52 passed, and tests/project 419 passed.

Worth a decision while the change is breaking anyway:

  • msg_reserved is already an int on BindingRefreshRequestMessage (pcapkit/protocols/data/internet/mh.py:171). After this PR it is bytes on the seven other messages, so one name carries two types.
  • Renaming the new field now is cheaper than doing it later. Otherwise the PR body should say why the name is shared.

@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.49% (unit tier, Python 3.14, c29c50a5a, 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 2043 89 636 25 94.77%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16719 218 4492 182 98.08%
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 78991b7 into main Oct 9, 2026
42 checks passed
@JarryShaw
JarryShaw deleted the fix/1437-mh-reserved-padding branch October 9, 2026 00:18
@JarryShaw JarryShaw moved this from In review to Done in PyPCAPKit Oct 9, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 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

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

Development

Successfully merging this pull request may close these issues.

1 participant