Skip to content

fix(mh): Message ID option always writes and reads a zero fraction - #1110

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1093-mh-mesg-id-fraction
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1093-mh-mesg-id-fraction

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Followed the coding style (make pylint, make mypy, make isort): ran with the Makefile flags on the two touched source files only; no new findings

  • make test passes, and a test case covers the change: ran the modules listed below, not the full suite

  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible: N/A, added centrally after the wave

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

Closes #1093

_make_opt_mesg_id now writes min(round(frac * 2**32), 0xFFFFFFFF). MesgIDOption.post_process now adds fraction / 2**32 seconds. timestamp is still a UTC datetime, and it now has sub-second precision. The unused math import is dropped from the schema module.

Probe at 1_700_000_000.5, before → after: fraction 2147483648000000 → 2147483648, bytes 0a08e8fe6f8000000000 → 0a08e8fe6f8080000000, read 22:13:20 → 22:13:20.500000.

Tests (new tests/protocols/internet/test_mh_mesg_id_fraction_unit.py): fractions 0, 0.5, 0.999999, an exact second, the clamp and the read scale.

  • With the fix: 4 passed, 3 subtests passed. Fix reverted: 4 failed, 2 passed (fraction 0 and the exact second pass either way).
  • test_mh_unit.py 52 passed, test_option_roundtrip_unit.py 6 passed, tests/project 379 passed, 1 skipped.

The NTP fraction field is a 32-bit binary fraction of a second.
`_make_opt_mesg_id` scaled it to microseconds and then multiplied by
2**32, so the packed field was always 0. It now writes
round(frac * 2**32), clamped to 0xFFFFFFFF. `MesgIDOption.post_process`
floored `fraction / 2**32` to 0 and now adds it as seconds, so
`timestamp` keeps sub-second precision.

Closes #1093
@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 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 1d1348342: GOOD TO GO (ran on Sonnet; author Opus)

  • Encoding: the fraction is written as round(frac*2**32), clamped to 32 bits, and read back as fraction/2**32 seconds. That matches the 32.32 NTP format in RFC 4285 §5, which mh.py cites.
  • Probe: 1_700_000_000.5 now packs …80000000 and reads back as .500000.
  • Round-trip: 20,000 random microsecond values all round-trip on this PR, and every one mismatches on main. Epoch 0 and pre-1970 values are also correct.
  • Tests: with the source reverted, 4 tests fail. test_mh_unit (52), test_option_roundtrip_unit and test_option_coverage_runtime pass.
  • Public field: timestamp is still a UTC datetime. Only its sub-second part changes.

Nit, pre-existing: the MesgIDOption.seconds comment says the epoch is 1970, but NTP counts from 1900. The code is right.

@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 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 84.75% (unit tier, Python 3.14, 1d1348342, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1048 2342 855 90.11%
pcapkit/corekit 1874 160 578 35 89.85%
pcapkit/dumpkit 136 25 40 6 76.70%
pcapkit/foundation 2422 312 842 82 84.19%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15652 1196 3942 366 89.91%
pcapkit/toolkit 487 109 144 9 74.64%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

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

@JarryShaw
JarryShaw merged commit cc737e5 into main Oct 6, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1093-mh-mesg-id-fraction branch October 6, 2026 20:30
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 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.

fix(mh): Message ID option always writes and reads a zero fraction

1 participant