protocols: size MH's MN-ID option from its subtype, not identifier's Python type

[JarryShaw]

Summary

MH._make_opt_mn_id sized the MN-ID option's identifier from the Python
type of the identifier argument, not from the subtype that actually
selects the wire format (mn_id_selector). For the IPv6_Address subtype —
the method's own default — the schema always packs a fixed 16-octet address
(IPv6AddressField ignores any declared length entirely), so a str or
int identifier declared a length that disagreed with what was packed.

Re-measured on faf86d26b (origin/main)

PR #437 rewrote this module wholesale (all 24 message types, 70/71 options,
4 CGA extensions) but did not fix this method — it only worked around
the defect in the test-fixture generator (examples/generators/options.py),
whose own comment names the bug:

The default identifier is the string '::', whose len() is 2 rather
than the 16 octets the field packs -- so the declared length is 3 going
out and 17 coming back. An address object avoids it.

Directly re-measuring _make_opt_mn_id against faf86d26b reproduces the
issue's table exactly:

identifier argument	declared length	packed octets	length + 2
'::' — the method's own default	3	19	5	mismatch
'2001:db8::1' (str)	12	19	14	mismatch
ipaddress.ip_address('2001:db8::1')	17	19	19	OK

After the fix, all three agree (length == 17, packed == 19,
length + 2 == 19).

Fix

• pcapkit/protocols/internet/mh.py: size id_len from the resolved
  subtype_val rather than from isinstance(identifier, ...). For
  Enum_MNIDSubtype.IPv6_Address, id_len is now always 16, and
  identifier is normalised to an ipaddress.IPv6Address first (mirroring
  the existing _make_opt_lmaa pattern in the same file), so the packed
  bytes and the declared length derive from one value instead of two
  independent computations. Other subtypes (NAI, and the BytesField
  fallback) were already sized correctly and are unchanged.
• examples/generators/options.py: removed the
  Enum_Option.MN_ID_OPTION_TYPE override that worked around the defect by
  forcing an already-correct-type identifier (ipaddress.IPv6Address('::1'))
  — the generator now exercises the real, now-fixed default ('::').
• docs/source/pep.rst: removed the "filed rather than fixed" bullet
  for MH MN-ID option sizes its identifier from the Python type rather than the subtype, so even the default arguments mis-declare length #448 under the Mobility Header section, now that it's fixed.

Siblings checked, none found

Only four _make_opt_* methods in mh.py take a subtype parameter:
_make_opt_mn_id, _make_opt_auth, _make_opt_mn_group, _make_opt_mag_id.
Checked each:

• _make_opt_auth sizes from len(data) where data is strictly typed
  bytes — no type union, no confusion.
• _make_opt_mn_group uses a fixed length=6 regardless of subtype.
• _make_opt_mag_id types identifier strictly as bytes (no union), and
  its schema (MAGIdentifierOption.identifier) is an unconditional
  BytesField, not a subtype-dispatched SwitchField like MNIDOption —
  so there's no type-vs-subtype mismatch possible there.

Grepped the whole file for isinstance(..., (IPv6Address|int))-based sizing;
the only other occurrences are unrelated (_seconds/lifetime helpers,
_make_opt_lmaa's address-family branch, which already normalises-then-sizes
correctly and was the style precedent for this fix). No sibling defect found.

Tests

tests/protocols/internet/test_mh_unit.py:

• New: test_mh_mn_id_option_length_matches_packed_octets — asserts
  len(packed) == length + 2 for every documented identifier type
  (str, int, bytes, IPv6Address) against the IPv6_Address subtype,
  including the method's own no-argument default.
• Fixed: test_mh_option_constructors_cover_known_options_and_dispatch
  had proto._make_opt_mn_id(Option.MN_ID_OPTION_TYPE, identifier=0x1234).length
  asserted equal to 3 — that was the buggy value under the default
  IPv6_Address subtype. Now asserts 17.

Before the fix (pristine faf86d26b, with these test edits applied): 4
subtests of the new test failed (3 != 17 / 12 != 17), plus the fixed
assertion failed on its own — 5 failed, 35 passed, 268 subtests passed
for test_mh_unit.py alone. After the fix: 36 passed, 0 failed, 272 subtests passed.

tests/protocols/test_option_roundtrip_unit.py: no EXPECTED_FAILURES
entry existed for mh-option/MN_ID_OPTION_TYPE before or after — it was
passing both times (363 subtests, 7 tests, unchanged), because the harness
was exercising the override's already-correct-type identifier rather than
the real default. Nothing in EXPECTED_FAILURES flipped. No new
_mh_option_overrides entry was needed; an existing one (a workaround for
this exact defect) was removed instead — see above.

Full suite (baseline faf86d26b)

Run with PYTHONSAFEPATH=1, fresh fixtures via
examples/generators/make_samples.py each time, same methodology both
sides (--collect-only counts, then full run):

	collected	passed	skipped	subtests passed
Before (pristine faf86d26b)	1012	995	17	1547
After (this fix)	1013	996	17	1553

No failures on either side — the pristine suite never caught this defect;
the delta is exactly the one new test method (6 new subtests) plus the
corrected assertion in the existing one.

Test plan

• Re-measured the defect directly against faf86d26b (table above)
• Confirmed protocols: complete the Mobility Header registry -- all 24 message types, 70 of 71 options, all 4 CGA extensions #437 did not fix it (only worked around it in the generator)
• Checked sibling _make_opt_* methods for the same confusion — none found
• Added a test that fails before and passes after (quoted above)
• Fixed a pre-existing assertion that encoded the buggy value
• Removed the now-unnecessary generator override, regenerated fixtures
• Ran full suite before and after with matching methodology

Closes #448

[JarryShaw]

What I read

gh api repos/JarryShaw/PyPCAPKit/pulls/464 (body) and gh api .../issues/448 (body). Neither
#464 nor #448 has any conversation comments (issues/464/comments, pulls/464/comments,
pulls/464/reviews all return length 0), so there was nothing else to read.

Reviewed at head 04e3cca92. The PR head has since moved to 5d869165f
("Merge branch 'main' into fix/issue-448-mh-mnid-length"). I verified independently — not just
taken on trust — that this is a pure pass-through merge of main (which is now 074ca2c09, #462):
git diff --stat 04e3cca92..5d869165f -- pcapkit/protocols/internet/mh.py tests/protocols/internet/test_mh_unit.py examples/generators/options.py docs/source/pep.rst is
empty, and git merge-base --is-ancestor 04e3cca92 5d869165f succeeds (no rebase, no rewrite). So
everything below at 04e3cca92 holds unchanged at 5d869165f.

The diff

git diff faf86d26b...04e3cca92 --stat (base is faf86d26b, the merge-base with origin/main):

docs/source/pep.rst                      |  3 ---
examples/generators/options.py           |  4 ---
pcapkit/protocols/internet/mh.py         | 11 +++++++-
tests/protocols/internet/test_mh_unit.py | 43 +++++++++++++++++++++++++++++++-

main has advanced to 074ca2c09 since (via 0283a6d59 #456 and 074ca2c09 #462), but neither
touches any of these four paths, so the rebase question is moot.

mh.py's change (pcapkit/protocols/internet/mh.py:7662-7670): the old
if isinstance(identifier, ipaddress.IPv6Address): id_len = 16 becomes
if subtype_val == Enum_MNIDSubtype.IPv6_Address: identifier = ipaddress.IPv6Address(identifier) [if not already]; id_len = 16. The elif isinstance(identifier, int) and
else: id_len = len(identifier) branches are untouched.

Item 1 — is sizing from subtype_val right for every subtype?

mn_id_selector (pcapkit/protocols/schema/internet/mh.py:341-356) special-cases exactly two
subtypes: NAI → StringField(length=pkt['length']-1), IPv6_Address → IPv6AddressField()
(fixed 16 octets, ignores declared length). Everything else — IMSI, P_TMSI,
EUI_48_address, EUI_64_address, GUTI, DUID, and any Reserved_*/Unassigned_* extension —
falls through to BytesField(length=pkt['length']-1).

I built the full subtype × identifier-type matrix directly against the fixed code (checked out at
04e3cca92 in my own worktree, pcapkit.__file__ asserted to start with the worktree path before
import):

NAI              identifier=str          -> declared=4  packed=6  match=True
NAI              identifier=bytes        -> PACK_FAIL: AttributeError: 'bytes' object has no attribute 'encode'
NAI              identifier=int          -> declared=3  PACK_FAIL: AttributeError: 'int' object has no attribute 'encode'
NAI              identifier=IPv6Address  -> CONSTRUCT_FAIL: TypeError: object of type 'IPv6Address' has no len()
IPv6_Address     identifier=str          -> declared=17 packed=19 match=True
IPv6_Address     identifier=bytes        -> declared=17 packed=19 match=True
IPv6_Address     identifier=int          -> declared=17 packed=19 match=True
IPv6_Address     identifier=IPv6Address  -> declared=17 packed=19 match=True
IMSI/P_TMSI/EUI_48/EUI_64/GUTI/DUID  identifier=bytes -> declared=7 packed=9 match=True   (all six agree)
IMSI/P_TMSI/EUI_48/EUI_64/GUTI/DUID  identifier=str   -> PACK_FAIL: struct.error: argument for 's' must be a bytes object
IMSI/P_TMSI/EUI_48/EUI_64/GUTI/DUID  identifier=int   -> declared=3  PACK_FAIL: struct.error: argument for 's' must be a bytes object
IMSI/P_TMSI/EUI_48/EUI_64/GUTI/DUID  identifier=IPv6Address -> CONSTRUCT_FAIL: TypeError: object of type 'IPv6Address' has no len()

So: the fix is correct and complete for the one thing it claims — IPv6_Address, the method's own
default subtype, now agrees for every documented identifier type (str, bytes, int,
IPv6Address), matching the PR's own re-measured table.

But the PR's "Siblings checked, none found" section says other subtypes "were already sized
correctly and are unchanged." That is true only for the type each field actually expects (str for
NAI, bytes for the fallback). The same type-vs-subtype confusion the issue describes survives
untouched in the elif isinstance(identifier, int) branch for every subtype except
IPv6_Address: it computes id_len from bit_length(), but neither StringField nor BytesField
has any int→bytes conversion in pre_process (pcapkit/corekit/fields/strings.py:80-94), so
.pack() raises rather than merely mis-declaring a length. This is not a regression — the int
branch is untouched by this diff, it is not exercised by any test before or after, and it is
outside what the PR and issue #448 scope themselves to (both are specifically about the
IPv6_Address default). I'm noting it because review question 1 asked directly whether the
confusion survives, and by this evidence it does, just in a spot this PR doesn't claim to have
looked at. Worth its own follow-up, not a blocker here.

One more data point: Schema_MNIDOption.identifier's own class annotation
(pcapkit/protocols/schema/internet/mh.py:723) is 'bytes | str | IPv6Address' — no int — while
the TYPE_CHECKING __init__ stub two lines below it says 'bytes | str | IPv6Address | int', and
_make_opt_mn_id's own signature also promises int. So int is a documented, not merely
hypothetical, input across every subtype, which is why I think this is worth a follow-up rather than
dismissing it as an unreachable corner.

Item 2 — is 3 → 17 justified?

Grepped the whole tree for MN_ID (tests/, pcapkit/, examples/) — the only other
_make_opt_mn_id/MNIDOption references are: a _read_opt_mn_id test building a schema by hand
with length=17 directly (unaffected, doesn't go through _make_opt_mn_id), and one already-correct
per-test override (ipaddress.ip_address(v6) at test_mh_unit.py:1917, unaffected). Nothing else in
the repo assumes the old value of 3.

Reproduced the PR's own before/after numbers rather than trusting them: checked out faf86d26b,
applied only the test-file hunk (git apply of git diff faf86d26b...04e3cca92 -- tests/protocols/internet/test_mh_unit.py), regenerated fixtures, ran
pytest tests/protocols/internet/test_mh_unit.py -q:

5 failed, 35 passed, 8 warnings, 268 subtests passed in 38.34s

— exact match to the PR's claimed "before" number for this file. Then restored to 04e3cca92 and
reran:

36 passed, 8 warnings, 272 subtests passed in 35.37s

— exact match to the PR's claimed "after". test_option_roundtrip_unit.py has no
mh-option/MN_ID_OPTION_TYPE EXPECTED_FAILURES entry either before or after
(git diff faf86d26b...04e3cca92 -- tests/protocols/test_option_roundtrip_unit.py is empty) — the
PR's claim on this file is correct too. The assertion change is justified: it was pinning a value
the issue itself documents as wrong, nothing else in the tree depends on the old value, and no
coverage is lost — the test still asserts a specific length, just the correct one now.

Item 3 — does removing the _mh_option_overrides entry change the fixture, and is the new one correct?

Counted the table myself rather than reusing the "ten" I'd been given (which I was separately told
was for the pre-#437 state anyway, not this tree): faf86d26b has 14 entries in
_mh_option_overrides, 04e3cca92 has 13 — exactly the one removal the diff shows
(Enum_Option.MN_ID_OPTION_TYPE).

Checking whether the capture actually changes cost me a real mistake worth recording: running
examples/generators/make_samples.py as a plain script does not put the worktree on sys.path
ahead of the venv's editable install (__editable__.pypcapkit-1.4.1.post2.finder.__path_hook__),
even under PYTHONSAFEPATH=1 — that flag only suppresses the implicit empty-string/cwd entry for
-c/-m/stdin, not the script's own directory, and the editable finder resolves pcapkit to
/local/home/jarryx/GitHub/PyPCAPKit/pcapkit (the main checkout, still unfixed) regardless of
which worktree the script lives in. I confirmed this directly: a probe script placed under
examples/generators/ and run the same way printed
MH resolved from: /local/home/jarryx/GitHub/PyPCAPKit/pcapkit/protocols/internet/mh.py. My first
two rounds of fixture generation were silently built against the wrong (unfixed) mh.py, which is
exactly the trap the task brief warned about. pytest itself is unaffected — a probe test
(import pcapkit; assert ...) confirmed pytest's own import resolves correctly to the worktree — so
this only corrupts fixture generation done by directly invoking the script, not test execution.
Fix: PYTHONPATH=<worktree-root> (not just PYTHONSAFEPATH=1) ahead of the script invocation, which
I verified puts the worktree first in sys.path and resolves pcapkit.__file__ correctly.

With that fixed, regenerating options-internet.pcap from each state and diffing the MN-ID option's
bytes directly:

• faf86d26b + override present → 08 11 02 + 16 octets ending ...01 (address ::1, from the
  override's ipaddress.IPv6Address('::1')), declared length 17.
• 04e3cca92 + override removed → 08 11 02 + 16 zero octets (address ::, the method's real
  default), declared length 17.

So yes, the capture changes — the MN-ID option's address value moves from ::1 to :: — and the
new one is correct: len(packed) == 19 == length(17) + 2 in both cases, it's only the address
value that changes, exactly as the PR's "the generator now exercises the real, now-fixed default"
claim says. test_option_roundtrip_unit.py/test_mh_unit.py/test_option_coverage_runtime.py
against the correctly-regenerated fixtures: 46 passed, 655 subtests passed — unaffected either way,
because mn_id_selector ignores the declared length for IPv6_Address regardless of which address
value is inside, so this option's roundtrip was never sensitive to this bug in the first place
(consistent with there never having been an EXPECTED_FAILURES entry for it).

Item 4 — unused imports in options.py?

The diff removes 4 lines (a comment + the MN_ID_OPTION_TYPE entry) and touches no import
statement. ipaddress (imported at examples/generators/options.py:804, inside
_mh_option_overrides) is still used once, at line 835, for
Enum_Option.Redirect_Mobility_Option's override. Confirmed by grep and independently by an AST
walk over the file (131 import bindings total, unaffected by this diff). No dangling import.

Item 5 — is the pep.rst removal accurate?

docs/source/pep.rst:223-225 (three lines: the #448 bullet and its two continuation lines) is
removed cleanly — the preceding bullet ("Payloads that belong to another protocol...") and the
following section ("Two wire-format traps are worth knowing...") read fine back-to-back with nothing
missing between them; I read the surrounding ~40 lines directly. Grepped the whole repo
(docs/, *.py, *.rst, *.md) for issues/448 / #448 — the only remaining hits are the fix's
own code comment and the new/edited test, which is provenance, not a dangling cross-reference. No
label or :ref: pointed at the removed bullet. The framing holds: pep.rst tracks feature requests,
this was a defect filed there for lack of anywhere else, and removing it now that it's fixed is
consistent with that scope.

CI

gh pr view 464 --json statusCheckRollup, checked repeatedly as jobs landed (head 5d869165f
throughout — headRefOid confirmed each time): all 20 real checks are SUCCESS
(Analyze, CodeQL, deploy-pages, Compat Python 3.10-3.15, Python 3.10-3.15, Integration Python
3.10-3.15), plus the two expected SKIPPEDs, which are not signs of trouble:
Docs test gate skips unconditionally on pull_request events
(.github/workflows/deploy-pages.yml:34, if: github.event_name != 'pull_request'), and
Gate (full suite, Python 3.14) is a gate-only-flagged reuse of the same reusable workflow used
by the regular Python 3.14 job that did run (.github/workflows/unit-tests.yml:163-164).
pyup.io/safety-ci reports state: SUCCESS (a StatusContext, not a pending Actions check, per the
brief's own caveat) — "No dependencies with known security vulnerabilities." Nothing red, nothing
still pending.

Full suite, measured myself, methodology note included

Fixtures regenerated fresh each time with PYTHONPATH=<worktree-root> forced (see item 3 for why
that matters), pcapkit.__file__ asserted to start with the worktree path before every measurement.
Both runs are full, not --collect-only substitutes.

	collected	passed	skipped	subtests passed
Before (faf86d26b, pristine)	1012	995	17	1547
After (04e3cca92)	1013	996	17	1553

Both exactly match the PR's own claimed numbers. "Before" took 852s, "after" 833s (wall clock varied
with contention from other concurrent worktree sessions on this host, as the task brief warned).
17 skipped both sides — RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') are all present
in this venv (HAS_RUNTIME true), so the 3 gated tests in test_option_coverage_runtime.py ran
rather than skipped; I did not investigate why the count is ever 20 elsewhere, since the brief
explicitly says that trigger is unexplained and not to invent a reason.

Verdict

GOOD TO MERGE, sha 04e3cca92 (and equally 5d869165f, its current head, by the empty-diff
pass-through-merge argument above).

The fix is correct and narrowly scoped to exactly what it claims: IPv6_Address sizing now derives
from subtype_val instead of Python type, verified across all four documented identifier types with
matching declared/packed lengths in every case. The test change, the override removal, the docs
removal, and the import hygiene all check out against independent measurement, not just the PR's own
narration. CI is fully green. The one thing I'd flag for a follow-up rather than as a blocker: the
elif isinstance(identifier, int) branch in the same method still has the type-vs-subtype confusion
issue #448 describes, for every subtype other than IPv6_Address — pre-existing, untouched by this
diff, unexercised by any test, and outside what #448/#464 scope themselves to, but real (demonstrated
above) and worth its own issue.