fix(mh): count a CGA extension's 4-octet header in its reported length

[JarryShaw]

Closes #512.
Closes #517.

One commit on top of c8fd97bcd. Both issues centre on pcapkit/protocols/internet/mh.py, so they land together.

#512 — every parsed CGA extension reported a length 2 octets short

MH._mh_option_length returns schema_length + 2, which is the right arithmetic for an RFC 6275 §6.2 mobility option: its Option Type and Option Length are one octet each. The three CGA-extension readers (_read_ext_none, _read_ext_multiprefix, _read_ext_exp) reused it, but a CGA extension's header is twice as wide — Schema_CGAExtension declares type: EnumField(length=2) and length: UInt16Field(), a 4-octet fixed header.

The root cause is the callers, not the helper. _mh_option_length is correct and is left untouched: a real mobility option gives _mh_option_length(2) == 4 == len(wire). What was wrong was routing a 4-octet-header structure through a 2-octet-header helper. So this adds a sibling _mh_extension_length returning + 4 and points the three readers at it, rather than changing the shared helper and breaking its 89 correct callers.

The write side was already right — _make_cga_extensions has always measured len(schema.pack()) — so only the read side disagreed with the wire.

RFC authority

The issue cites RFC 4866 and RFC 3972 §3. Neither is the right source, and the docstrings cite RFC 4581 §2 instead:

• RFC 4581 §2 is what actually defines the extension TLV, and it formally updates RFC 3972 (Updates: 3972 in its header): "Extension Type: 16-bit identifier of the type of the Extension Field." / "Extension Data Length: 16-bit unsigned integer. Length of the Extension Data field of this option, in octets." Two 16-bit fields — a 4-octet header.
• RFC 3972 §3 only reserves an opaque extension region ("an optional variable-length field that is not used in the current specification") and gives it no Type/Length structure at all.
• RFC 4866 has zero hits for this TLV. It defines Mobile IPv6 mobility header options (Option Type/Option Length), a different structure.
• RFC 5535 §5 corroborates from the other direction: its Ext Len is the "[l]ength of the Extension in octets, not including the first 4 octets."

#517 — the three follow-ups

1. _read_opt_pad still open-coded clen + 2. Converted to self._mh_option_length(clen), which is what refactor: extract duplicated wire-unit arithmetic across mh/ipv6_route/hopopt/ipv6_opts #509 created the helper to collect. hopopt.py and ipv6_opts.py had been converted; only the mh.py branch was left behind, so the arithmetic still existed twice in the file that introduced the helper.
2. The dangling cross-reference. _mh_option_length's docstring referred the reader to "the surviving explanation of that fix … below", but that Note: lived only in hopopt.py and ipv6_opts.py — the paragraph was copied and its "below" did not travel with it. _read_opt_pad now carries the Note:, explaining why Pad1 is the helper's sole exception, and the cross-reference points at it.
3. The wrong RFC section in ipv6_opts.py:467. Now cites RFC 8200 §4.2, not the §4.6 the issue asks for. §4.2 ("Options") is where the sentence the docstring quotes verbatim — "Length of the Option Data field of this option, in octets" — actually appears; §4.6 is the Destination Options header this class implements, and it cross-references §4.2 rather than restating the arithmetic. The issue anticipated this: "(§4.2 is arguably the right citation for both, since it defines the option format itself.)" The docstring now names §4.6 as the header and §4.2 as the format, so both facts are on the page.

Fails-without evidence

Environment: .venv/bin/python 3.14.7, PYTHONSAFEPATH=1 PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=<worktree>, with pcapkit.__file__ asserted to resolve inside the worktree rather than the editable install. pytest-subtests is not installed; pytest 9.1.1's native subtests report a failing subtest's parent as PASSED, so the exit code is the signal below, not the summary line.

#512 — library reverted to c8fd97bcd, new tests kept:

FAILED  test_mh_cga_extension_length_counts_the_four_octet_header
        AttributeError: type object 'MH' has no attribute '_mh_extension_length'
FAILED  test_mh_option_and_extension_length_helpers_do_not_share_a_header_width
SUBFAILED ×9  test_mh_experimental_cga_extensions_round_trip
              (codes Exp_FFFD/FFFE/FFFF × sizes 0/1/16)
11 failed, 1 passed, 40 deselected     EXIT=1

#517 item 1 — same reverted library:

FAILED  test_mh_pad_option_reader_delegates_to_the_option_length_helper
        AssertionError: 7 != 1005
1 failed, 42 deselected     EXIT=1

Fix restored, all four:

4 passed, 39 deselected, 30 subtests passed     EXIT=0

What cannot be proved this way, stated plainly

• #509 follow-ups: one call site left open-coded, a dangling cross-reference, and a wrong RFC section #517 items 2 and 3 are a docstring and a citation. No test can fail on them. There is no assertion that a Note: exists or that an :rfc: role names §4.2; claiming a red/green proof for them would be manufacturing one. They are reviewed by reading, not by running.
• #509 follow-ups: one call site left open-coded, a dangling cross-reference, and a wrong RFC section #517 item 1 is behaviour-neutral by construction — clen + 2 and _mh_option_length(clen) return the same number, which is why test_mh_padding_options_parse_from_the_wire passed throughout and why refactor: extract duplicated wire-unit arithmetic across mh/ipv6_route/hopopt/ipv6_opts #509 was verified byte-identical. So the test pins the delegation rather than the value: it patches _mh_option_length with a sentinel (lambda n: 1000 + n) and asserts the reader follows it. An open-coded branch ignores the patch and answers 7; a converted one answers 1005. That is what the issue actually asks about, and it does fail against unfixed code.
• Pad1 is asserted to stay at 1 under the same patch, pinning the documented exception against a future "simplification" that routes it through the helper too.

Regression surface

Run	Before (c8fd97bcd)	After
tests/protocols/internet/test_mh_unit.py	40 passed, 424 subtests	43 passed, 445 subtests
tests/protocols/internet	—	191 passed, 654 subtests, EXIT=0
tests/protocols/test_option_roundtrip_unit.py	—	6 passed, 358 subtests, EXIT=0

EXPECTED_FAILURES in tests/protocols/test_option_roundtrip_unit.py cannot be grepped or AST-parsed (it uses ** unpacking), so it was imported and inspected directly: 59 entries, keyed <protocol>-<kind>/<NAME>, zero of them MH-related and zero extension-related. CGA extensions are not in the option/parameter registries that file iterates, so no entry flips and that file is untouched.

Call-site accounting for self._mh_option_length(: 92 → 90. Three removed (the CGA readers) and one added (_read_opt_pad). Verified by counting occurrences rather than lines — grep -c counts lines and misses this.

Out of scope

• hopopt.py:456 carries the same citation imprecision as #509 follow-ups: one call site left open-coded, a dangling cross-reference, and a wrong RFC section #517 item 3, citing RFC 8200 §4.3 for the quoted Opt Data Len sentence that lives in §4.2. §4.3 is the correct header for HOPOPT, so it is wrong in a milder way than ipv6_opts.py was — but the quote still comes from §4.2. Not touched here; worth its own issue.
• pcapkit/protocols/schema/internet/mh.py and schema/internet/ipv6_opts.py are read-only inputs here. The schema is the evidence for the 4-octet header, and it is already correct — the defect was entirely in the protocol layer's interpretation of it.
• No coverage run: the three new tests and one corrected test cover the new _mh_extension_length and the converted _read_opt_pad branch, but the number is not measured here.

[JarryShaw]

✅ GOOD TO MERGE — the two length helpers (_mh_option_length +2, _mh_extension_length +4) are proven correct against the actual schema field widths (Option = 1+1 octets, CGAExtension = 2+2 octets), the RFC 4581 §2 and RFC 8200 §4.2 citations both check out against the real RFC text, and no other 4-octet-header caller was left on the 2-octet helper anywhere in mh.py.

[JarryShaw]

Detailed review (independent verification, falsify-not-bless)

Reviewed at head 5d3558f76ee24fac79f0b291531564b998e141d0 (one commit on top of c8fd97bcd) in an isolated worktree.

1. Callers were wrong, not the helper — confirmed from the schema itself, not the PR's prose.

• pcapkit/protocols/schema/internet/mh.py Option: type: EnumField(length=1) + length: UInt8Field() → 2-octet header. _mh_option_length's +2 is correct.
• CGAExtension: type: EnumField(length=2) + length: UInt16Field() → 4-octet header. New _mh_extension_length's +4 is correct.
• Exactly 3 call sites for _mh_extension_length (_read_ext_none, _read_ext_multiprefix, _read_ext_exp) — matching the only three CGAExtension subclasses that exist (UnknownExtension, MultiPrefixExtension, ExperimentalExtension). No CGA extension caller left on the old helper.
• Every other suboption/attribute family sharing the call pattern (FlowIdentificationSuboption, ANISuboption, LMAControlledMAGSuboption, QoSAttribute) was checked directly against its own schema fields — all genuinely 2-octet headers, correctly still on _mh_option_length. CGAExtension is the only 4-octet-header structure in the file.
• Write side (_make_cga_extensions) computes data_len = len(schema.pack()) — genuinely measuring packed wire bytes including the 4-octet header, confirming only the read side had disagreed with the wire.

2. RFC 4581 §2 vs RFC 4866 / RFC 3972 §3 — the PR is right. Read the actual RFC text:

• RFC 4581 (Updates: 3972) §2 requires TLV fields, with Extension Type and Extension Data Length each defined as 16-bit — a 4-octet header, matching the PR's arithmetic.
• RFC 3972 §3's Extension Fields region is explicitly "an optional variable-length field that is not used in the current specification" — no Type/Length subfields, no TLV structure at all. Zero hits for the claimed format.
• RFC 4866 has zero occurrences of "Extension Type"/"Extension Data Length" anywhere; its only option format is the unrelated 8-bit-Type/8-bit-Length mobility option. Zero hits, confirmed.
• RFC 5535 §5's "Ext Len... not including the first 4 octets" corroborates the 4-octet header from the opposite direction.

3. RFC 8200 §4.2 vs §4.6 (what issue #517 asked for) — the PR's citation is correct for the specific claim. Fetched RFC 8200 §4.2, §4.3, and §4.6 directly: §4.2 ("Options") is exactly where the quoted sentence "the length of the Option Data field of this option, in octets" lives. §4.6 ("Destination Options Header") defines the container that carries the TLVs (Hdr Ext Len measured in 8-octet units) but contains no such sentence; §4.3 (Hop-by-Hop, structurally parallel to §4.6) doesn't either. Dismissing the literal §4.6 request was the right call — §4.6 is what the class implements, §4.2 is where the specific arithmetic being quoted is actually defined, and the PR's docstring update names both correctly. The related, out-of-scope citation at hopopt.py:456 (§4.3 quoted for the same sentence) carries the identical imprecision — correctly deferred, not silently missed.

4. Fails-without-fix, reproduced independently. Reverting only pcapkit/protocols/internet/mh.py to c8fd97bcd (keeping the new/modified tests):

• Targeted selection (cga_extension_length, option_and_extension_length_helpers, pad_option_reader_delegates): 3 failed, exit 1 — AttributeError: type object 'MH' has no attribute '_mh_extension_length' and AssertionError: 7 != 1005.
• test_mh_experimental_cga_extensions_round_trip alone: 9 failed, 1 passed, exit 1 — 9 SUBFAILED entries, with the parent test line printed as "1 passed" in the summary despite exit 1 (the pytest-9.1.1 subtest-mislabeling quirk, personally reproduced).
• Restored: tests/protocols/internet/test_mh_unit.py → 43 passed, 445 subtests, exit 0. tests/protocols/test_option_roundtrip_unit.py → 6 passed, 358 subtests, exit 0. Both exact matches to the PR's claimed after-state.
• Call-site count: _mh_option_length calls go from 92 (on c8fd97bcd) to 90 (at PR head) — counted by occurrence, not by line, so this correctly includes the one call site #509 follow-ups: one call site left open-coded, a dangling cross-reference, and a wrong RFC section #517 item 1's _read_opt_pad conversion added alongside the three CGA readers that moved off it. EXPECTED_FAILURES (imported directly, not grepped) has 59 entries, 0 MH-related — confirmed untouched.

5. Falsification attempts — no wrong implementation found that slips past the shipped tests.

• Hardcoding +4 inline at one call site instead of a real helper: caught — test_mh_option_and_extension_length_helpers_do_not_share_a_header_width calls MH._mh_extension_length(4) directly and asserts == 8; no such method raises AttributeError.
• Fixing only 2 of the 3 CGA readers: caught — test_mh_cga_extension_length_counts_the_four_octet_header exercises all three readers individually plus the dispatcher, each asserting parsed.length == len(packed).
• Changing the shared _mh_option_length to +4 instead of adding a new helper: caught — the same test pins _mh_option_length(x) == x + 2 across 7 values plus a real mobility-option round trip, and the full 43-test/445-subtest mh.py suite (covering ~90 real option call sites) would break en masse.

What could not be independently improved on

Nothing outstanding. One incidental finding during fixture generation (a SchemaWarning for small CGA-extension payload sizes in make_samples.py) was checked and found to be generic sample-generator behavior affecting several unrelated protocols in the same run, not something introduced by this PR's arithmetic — noted, not a defect.

Verdict

All five load-bearing claims (callers-not-helper, RFC 4581 authority, RFC 8200 §4.2 judgment call, fails-without-fix proof, and completeness of the caller migration) independently reproduce. Recommend merge.