fix(schema): read the MPTCP option length from bit 8, not bit 1 (#553)

[JarryShaw]

Closes #553.

The defect

pcapkit/protocols/schema/transport/tcp.py declared _MPTCP's option-header
forward match as:

test: 'MPTCPSubtypeTest' = ForwardMatchField(BitField(length=3, namespace={
    'length': (1, 8),
    'subtype': (16, 4),
}))

BitField's namespace entries are (start_bit, bit_width), sliced out of one
big-endian bit string over the whole 3-octet window. length read eight bits
starting one bit in, straddling the low seven bits of kind and the high bit
of length.

The offset arithmetic, checked against RFC 8684

RFC 8684 section 3 lays every Multipath TCP option out as Kind (8 bits),
Length (8 bits), Subtype (4 bits), then subtype-specific content. Numbering
from the option's first octet that puts Length at bits 8-15.

Three things confirm the window really is anchored at kind, rather than after
the option header:

1. The sibling entry 'subtype': (16, 4) is already written against that
   numbering, and decodes correctly today. Two entries in one namespace cannot be
   anchored differently — BitField.post_process slices both from the same bit
   string — so subtype at bit 16 fixes the origin at kind and leaves bit 8 as
   the only place length can begin.

2. _MPTCP is registered as the schema for the option itself
   (Option.register(Enum_Option.Multipath_TCP, _MPTCP)) and, alone among the
   option schemas, does not inherit Option — so it is handed the option's
   first octet and nothing has consumed kind/length before it.

3. Measured, on the 12-octet MP_CAPABLE of RFC 8684 figure 4, whose first three
   octets are 1e 0c 01 → bits 000111100000110000000001:

   slice	value
   [1:9] — old (1, 8)	60
   [8:16] — new (8, 8)	12
   [16:20] — subtype	0 = MP_CAPABLE

So (8, 8), and the issue's reported "60 instead of 12" reproduces exactly.

Why nothing caught it

The decoded length is used for one thing — SchemaField(length=pkt['test']['length'])
in mptcp_data_selector, sizing the nested subtype schema. Over-declaring that
size fails silently: the enclosing OptionField stops at the end-of-option-list
marker or the declared options length, so the over-read is absorbed rather than
reported.

Coverage

New tests/protocols/transport/test_tcp_mptcp_length_unit.py, five cases read off
the schema's own declared field (_MPTCP.__fields__['test']) rather than a copy,
so a regression in the shipped declaration is what fails. Includes the exhaustive
case: all 256 lengths a single octet can declare must decode back to themselves.

Proven to fail without the fix — pytest exit code read from a file, since a
wrapper's exit code is not pytest's:

# with 'length': (1, 8)
PYTEST_EXIT=1
3 failed, 2 passed
AssertionError: 60 != 12
# with 'length': (8, 8)
PYTEST_EXIT=0
5 passed

The two cases that pass either way are the subtype and anchor invariants, which
are correct before and after — they exist to rule out "fixing" the length by
moving the window instead of the entry.

Measured against /local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python with
PYTHONSAFEPATH=1 and pcapkit.__file__ asserted into this worktree.

Scope: #541 and the other seven MPTCP makers

#553 asks whether the eight tcp-mptcp/* entries in EXPECTED_FAILURES share
#541's root cause, so that #541 could close here. They share one structural
cause but need three different fixes, and one of them is unrelated — so this PR
deliberately does not attempt the family, and #541 should stay open. Measured, by
calling each of the eight makers and reading the exception:

The structural cause: every MPTCP subtype schema inherits MPTCP
(schema/transport/tcp.py:608), not Option, and MPTCP declares kind,
length and subtype only under if TYPE_CHECKING: (lines 613-619) — pure
annotations, no field descriptors. Their per-class TYPE_CHECKING __init__ stubs
nonetheless advertise kind/length, which is what leads every
_make_mptcp_* helper to pass them; both are then dropped with
UnknownFieldWarning.

It surfaces three ways:

Group	Subtypes	Failure
A	MP_CAPABLE, ADD_ADDR (#541), REMOVE_ADDR, MP_PRIO	KeyError: 'length' in pack(), from each schema's own pkt['length'] predicate
B	DSS, MP_FAIL, MP_FASTCLOSE	AttributeError: no attribute 'kind', deferred to _read_tcp_options — pack() succeeds
C	MP_JOIN	AttributeError: 'TCP' object has no attribute '_flags' — never reaches schema construction at all

Group C is a genuinely separate defect: _make_mptcp_join branches on
self._flags, which only exists while parsing. It masks whether the kind/length
cause even applies to MP_JOIN.

Fixing the family means giving the subtype schemas real kind/length fields (or
adding a construct-path wrapper mirroring _MPTCP), which changes ten schema
classes and both directions of the MPTCP path — a much larger change than a bit
offset, and one that would swamp this diff. Worth noting it is needed for parsing
too, not just construction: a real 12-octet MP_CAPABLE does not parse on
main either, raising KeyError: 'length' from MPTCPCapable.rkey's
pkt['length'] != 32 predicate, independently of which bit offset the header
used. That is why this PR's assertion is on the decoded namespace rather than on
an end-to-end parse.

EXPECTED_FAILURES is untouched — all eight entries still fail in the
recorded way, and tests/protocols/test_option_roundtrip_unit.py passes
unchanged.

Verification

• Unit tier (--ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'), the selection CI runs: 1104 passed, 8 skipped, identical to the pre-change baseline on main.
• The only failures anywhere are in the fixture-dependent tier, which needs make samples and fails identically on main (134 failed / 1165 passed both before and after).

[JarryShaw]

✅ GOOD TO MERGE — independently derived the correct bit offset from RFC 8684 section 3's own layout (Kind at bits 0-7, Length at bits 8-15, Subtype at bits 16-19) and hand-decoded the worked example's raw bytes (1e 0c 01) bit-by-bit, confirming both the old offset's wrong answer (60) and the new offset's right answer (12) exactly as claimed. Confirmed the new test genuinely exercises the wire format via .unpack() on real bytes rather than a self-consistent round-trip, reproduced the fails-without proof exactly (3 failed/exit 1 reverted, 5 passed/exit 0 fixed), and additionally falsified with a plausible off-by-one wrong fix ((9, 8)) to confirm the test suite is genuinely sensitive rather than coincidentally satisfied. Confirmed the eight tcp-mptcp/* EXPECTED_FAILURES entries and tests/protocols/test_option_roundtrip_unit.py are correctly left untouched — nothing in the current suite exercises the buggy decode path on a currently-passing route, since every other MPTCP-touching test constructs its packet dict with a hand-supplied length= value rather than decoding real bytes through the fixed field.

[JarryShaw]

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

Head sha reviewed: 6c9eab6c61d00dcc542273a8ee6f6bd9fe6f3d85.

Is (8, 8) actually right, or merely different from (1, 8)?

Derived the offset from RFC 8684 section 3 myself, independent of the PR's own citation: every Multipath TCP option is laid out as Kind (8 bits), Length (8 bits), Subtype (4 bits) plus subtype-specific content, numbered from the option's own first octet. That puts Length at bits 8-15, matching the fix.

Then hand-decoded the actual worked example rather than trusting the claimed numbers: RFC 8684 figure 4's 12-octet MP_CAPABLE begins 1e 0c 01, i.e. the 24-bit string 000111100000110000000001. Slicing it myself:

• Old [1:9] (8 bits from position 1): 00111100 = 60.
• New [8:16] (8 bits from position 8): 00001100 = 12.
• subtype [16:20]: 0000 = 0 = MP_CAPABLE.

Both match the PR's claim exactly, and independently confirm the fix is correct, not merely different. I also checked the "eleven extra lines" the diff ratio implied might be padding: they are entirely a source-code comment on the test field explaining the anchor and the RFC citation, in the same style used elsewhere in this file — not hidden logic, not padding.

Does the fix change decoded length for other MPTCP subtypes, and did anything depend on the old value?

The same test field fronts every subtype, so the fix changes the decoded length uniformly across all of them. Checked whether anything currently passing depends on the old (wrong) value: mptcp_data_selector (pcapkit/protocols/schema/transport/tcp.py:188-213) is the only consumer of the decoded length, using it solely to size the nested SchemaField. Grepped every other test file that touches MPTCP (test_fields_ipaddress.py, test_fields_misc.py, test_tcp_udp_unit.py, test_protocols.py) and found that every one of them constructs its packet dict with a hand-supplied 'length': 12 (or similar) rather than decoding real wire bytes through _MPTCP's BitField -- so none of them exercises the buggy offset at all, before or after this fix. Combined with the PR's own Group A/B/C analysis (independently spot-checked: test_option_roundtrip_unit.py's 8 tcp-mptcp/* entries all fail before pack() ever reaches the length-consuming code), there is no currently-passing route that depended on the wrong offset, and none that could regress.

Does the test exercise the wire format, or just round-trip?

Read tests/protocols/transport/test_tcp_mptcp_length_unit.py in full. decode_header() calls field({}).unpack(option, {}) directly on real bytes -- genuine wire-format decoding, not a construct-then-repack round-trip that a consistently-wrong offset could satisfy in both directions. The field itself is read off the shipped schema (_MPTCP.__fields__['test']), not copied, so a future regression in the real declaration is what the test would catch. The exhaustive 256-case sweep specifically targets the failure mode the coordinator was worried about: with kind fixed at 0x1e (binary 00011110), any wrong offset that happens to give the right answer for one or two hand-picked lengths would still have to survive all 256 -- which is exactly the coincidence the old bug exploited on the two RFC-figure examples this file's own docstring names.

Fails-without, reproduced exactly, exit code read from a file:

• Reverted pcapkit/protocols/schema/transport/tcp.py to main, ran the new test file: exit 1, 3 failed, 2 passed, AssertionError: 60 != 12 -- matches the PR's claimed table precisely.
• Restored, clean run: exit 0, 5 passed.

Additional falsification beyond what the PR itself did: tried a plausible off-by-one wrong fix, 'length': (9, 8) instead of (8, 8) -- a mistake someone might make by miscounting from 0 vs 1. Result: exit 1, 3 failed, AssertionError: 24 != 12. The test suite is genuinely sensitive to more than just the one specific bug being fixed, not merely tuned to flip on this exact change. Reverted; git diff <head> --stat empty before moving on.

Did it stay inside its lane?

File list confirms only pcapkit/protocols/schema/transport/tcp.py and the new test file were touched -- tests/protocols/test_option_roundtrip_unit.py (owned by nobody) is untouched, confirmed by git diff origin/main 6c9eab6c6 -- tests/protocols/test_option_roundtrip_unit.py producing no output. The PR does not stay silent about whether it should have affected an EXPECTED_FAILURES entry -- it explicitly states "EXPECTED_FAILURES is untouched -- all eight entries still fail in the recorded way," which I independently confirmed by running tests/protocols/test_option_roundtrip_unit.py on the PR head (6 passed, 358 subtests, exit 0) and by importing EXPECTED_FAILURES directly to count the tcp-mptcp/* keys (8, matching the PR's table exactly: ADD_ADDR, DSS, MP_CAPABLE, MP_FAIL, MP_FASTCLOSE, MP_JOIN, MP_PRIO, REMOVE_ADDR).

Also confirmed #554 was not folded into this PR -- the diff is exactly the 2 files stated (pcapkit/protocols/schema/transport/tcp.py, the new test), nothing touching whatever #554's scope covers.

CI status

Per the coordinator, CI on this head is running (4 SUCCESS/0 failing/2 SKIPPED/17 running of 23 at last check) and not yet finished; not waited on, per standing instruction. Re-tallying a CI number is not attempted here beyond what the coordinator already reported, per the recent correction about __typename-filtering CheckRuns vs. StatusContexts.

What remains unverified

• The full unit-tier suite (1104 passed, 8 skipped claimed) is running in the background as I post this and had not completed; I will post the exact number as a follow-up once it finishes, having learned from an earlier mistake in this session not to touch the worktree checkout while a background test run depends on it.
• The Group A/B/C analysis of the other seven MPTCP makers (why TCP._make_mptcp_addaddr cannot pack: kind/length rejected as fields, then the port predicate KeyErrors on length #541 should stay open) was read and spot-checked against mptcp_data_selector and the roundtrip test's pass/fail state, but I did not independently call each of the eight _make_mptcp_* helpers myself to re-derive the exact exception type table.