fix(tcp): give MPTCP real kind/length fields, repairing pack and parse

[JarryShaw]

Summary

Fixes #541. TCP._make_mptcp_addaddr could not build an ADD_ADDR option end to end: its kind=/length= arguments were rejected with UnknownFieldWarning and dropped, and .pack() then raised KeyError: 'length' from the port field's own condition, pkt['length'] in (10, 22).

Root cause, deciding between the two the issue names: MPTCP, the base class every Multipath TCP subtype schema inherits, declared kind and length only under typing.TYPE_CHECKING -- annotations for a type checker, not real fields -- unlike Option, which every non-Multipath TCP option schema inherits instead. Schema.__update__ therefore rejected both keyword arguments as unknown. The port predicate's apparent circularity (length looks like it depends on whether port is present, and port is gated on length) is not an independent problem: length is computed by the caller before packing starts, so once it is a real field that lands earlier in field order than port, the predicate simply reads a value that is already there. Fixed by declaring real kind/length fields on MPTCP itself, mirroring Option.

This repairs unpacking as well as packing, and for the identical reason -- confirmed independently by cross-review. With no kind/length fields ahead of it, MPTCPAddAddress's own leading field (test, the subtype/IP-version octet) read the kind octet itself rather than the third octet of the option -- an off-by-two in field alignment, not a wire-format change: a correct sender's octets were always right, only this library's reading of them was shifted. Hand-built, spec-correct ADD_ADDR and MP_PRIO octets (from the RFC 8684 figures, built through no _make_mptcp_* maker) failed to parse on main -- FieldError: TCP: [OptNo 30] 3 invalid IP version for ADD_ADDR (0x1e's low nibble decoded as IP version 14) and KeyError: 'length' for MP_PRIO (the same exception #541 was filed against, reached by parsing instead of packing) -- and now parse correctly, every field exactly as built.

Scope: that one change also fixes construction for the other ten _make_mptcp_* helpers that pass kind=/length= the same way -- _make_mptcp_unknown, _make_mptcp_capable, _make_join_syn, _make_join_synack, _make_join_ack, _make_mptcp_dss, _make_mptcp_remove, _make_mptcp_prio, _make_mptcp_fail, _make_mptcp_fastclose -- confirmed by hand for each (all now construct and pack without error). Left alone, as distinct pre-existing defects this fix exposes rather than causes, and filed rather than fixed here:

• _make_mptcp_join still raises AttributeError: 'TCP' object has no attribute '_flags' -- it reads parser-only state, so it cannot be called outside a real parse. Unaffected by this change either way.
• MPTCP.subtype is declared only under TYPE_CHECKING, so every subtype built via TCP's convenience constructor raises AttributeError #566 -- building any MPTCP subtype through the TCP(options=[(Enum_Option.Multipath_TCP, {...})], ...) convenience constructor (as opposed to calling _make_mptcp_* directly, or parsing real wire octets, both of which this PR fixes) still fails, with AttributeError: '<Class>' object has no attribute 'subtype'. That constructor immediately reads options back through _read_mptcp_* off the in-memory schema object without ever going through a byte-level parse, and .subtype is only ever populated by _MPTCP.post_process(), which runs on a real unpack. Same category of defect as kind/length (an attribute _read_mptcp_* expects that direct construction never populates), but a distinct fix.
• TCP._make_mptcp_capable writes length 20/32 where RFC 8684 gives 12/20, and now packs wrong bytes rather than crashing #567 -- _make_mptcp_capable's length arithmetic is swapped (length=20 if rkey is None else 32, where RFC 8684 section 3.1 wants 12/20) -- previously masked entirely by the KeyError this PR fixes, now exposed as wrong-but-non-crashing packed bytes. The more dangerous of the two, since it packs successfully rather than raising. (Cross-review additionally found MPTCPCapable.rkey's own predicate, pkt['length'] != 32, contradicts that arithmetic in the opposite direction -- filed as an amendment to TCP._make_mptcp_capable writes length 20/32 where RFC 8684 gives 12/20, and now packs wrong bytes rather than crashing #567, not fixed here either.)

tests/protocols/test_option_roundtrip_unit.py's EXPECTED_FAILURES table (owned by nobody, decision made by the maintainer rather than by me): this fix moves 7 of the 8 tcp-mptcp/* entries (all but MP_JOIN) from KeyError: 'length' / no attribute 'kind' to a uniform no attribute 'subtype', still at the CONSTRUCT stage. Re-pointed rather than deleted: each entry's fragment now quotes the exact class name the current run raises against (e.g. "'MPTCPCapable' object has no attribute 'subtype'"), and each defect points at #566 -- MP_CAPABLE's also names #567, since that case would still pack the wrong length even once #566 is fixed. MP_JOIN (no attribute '_flags') is untouched. tests/protocols/test_option_roundtrip_unit.py::OptionRoundTripTests::test_round_trip_is_identity_or_a_recorded_gap passes again as a result.

Test plan

• Added tests/protocols/transport/test_tcp_mptcp_addaddr_unit.py:
  • Pack direction: packed bytes for ADD_ADDR on IPv4/IPv6 x with/without port, plus the opt= re-entry path. Verified each fails with the filed UnknownFieldWarnings and KeyError: 'length' on the pre-fix code, and passes after it.
  • Parse direction: hand-built, spec-correct ADD_ADDR and MP_PRIO octets (built through no maker) parsed into a TCP and asserted field by field. Verified each fails on the pre-fix code with FieldError: TCP: [OptNo 30] 3 invalid IP version and KeyError: 'length' respectively, and passes after it.
• pytest tests/protocols/transport -- 90 passed, 431 subtests passed (samples regenerated via examples/generators/make_samples.py).
• pytest tests/protocols/test_option_roundtrip_unit.py -- 6 passed, 358 subtests passed, after re-pointing the seven EXPECTED_FAILURES entries above.
• pytest tests (full tier) -- 0 failed.
• python util/changelog_md.py --check -- clean after regenerating CHANGELOG.md from docs/source/changelog/1.5.0.rst.

[JarryShaw]

❌ NEEDS CHANGES

Cross-model review (Opus 5; the PR was authored on Sonnet), head 45d37ad81.

The code change is correct, and it fixes considerably more than this PR claims. That is the headline finding and I proved it independently: on main, hand-built spec-correct ADD_ADDR wire bytes do not parse at all (FieldError: TCP: [OptNo 30] 3 invalid IP version, because MPTCPAddAddress.test was reading the kind octet 0x1e and deriving version = 14), and spec-correct MP_PRIO bytes raise KeyError: 'length'. Both parse correctly on this branch, with every field value exactly as constructed. MPTCP unpacking was broken in the same way and for the same reason as the packing, and this PR repairs both directions. It is a correction of an existing off-by-two, not a wire-format shift — which was the main risk I set out to falsify.

Two changes needed, both about what this PR documents and tests rather than what it does:

1. Add a parse-direction regression test. The new test file is pack-only — all five cases call schema.pack(), and nothing constructs a TCP from bytes. (test_opt_argument_round_trips_through_the_maker is maker→maker despite the name; it passes a previously-parsed opt back in, with no byte-level parse.) So the unpack repair — a real behaviour change I measured on both trees — ships with nothing pinning it, in a repository whose standard is that every fix proves it fails without itself. Feeding back the bytes your own tests already assert (e.g. 1e0a34010102030401bb) through TCP(io.BytesIO(...)) and checking subtype/addr_id/address/port would lock it in, and fails on main.

2. The changelog entry covers only the packing direction. It describes the maker, the UnknownFieldWarnings and the KeyError, and stops. "MPTCP options from a real capture now parse" is the more user-visible half, and a user whose capture failed on 1.5.0b4 will not find this entry. One or two sentences.

The amend from 9e93c7772 to 45d37ad81 is clean on all four counts it had to be. Source diff unchanged — git diff 9e93c7772 45d37ad81 touches only tests/protocols/test_option_roundtrip_unit.py (+40/−21). Fragments are per-class ("'MPTCPCapable' object has no attribute 'subtype'" and so on) rather than a shared "no attribute 'subtype'" that would have matched all seven and pinned none — exactly what Gap.fragment's docstring demands. MP_CAPABLE names both #566 and #567. And the prose asserts no more than was measured: the 4-with-KeyError / 3-with-no attribute 'kind' split matches the entries it replaces, and I confirmed both cited line numbers land exactly — schema/transport/tcp.py:650 is the subtype annotation inside if TYPE_CHECKING:, and transport/tcp.py:2669 is the length=20 if rkey is None else 32.

I did not treat #566's and #567's absence as a finding — deferring them was right. But one finding on #567 itself, worth acting on while it is fresh rather than a change to this PR: its description says the maker's 20/32 is RFC 8684's 12/20 "swapped", and fixing only that would leave the defect half-live. MPTCPCapable.rkey's own predicate is lambda pkt: pkt['length'] != 32, which is independently wrong and directly contradicts the maker — the maker signals rkey present with the one value that tells the schema to drop it. Measured on this branch: length=32 with an rkey packs 12 octets with the receiver key silently gone; length=12 (the RFC value) with no rkey packs 20 octets carrying a phantom all-zero key. So the schema cannot express a 12-octet MP_CAPABLE at all, and #567 needs to cover the predicate too.

Everything I ran is green: transport tier 82 passed / 73 subtests (exit 0, matching your numbers exactly), roundtrip 6 passed / 358 subtests (exit 0), changelog drift exit 0, and all five new tests fail with the documented 'kind'/'length' UnknownFieldWarnings when I revert the fix (exit 1). Detail below.

[JarryShaw]

Detailed cross-review — #565 @ 45d37ad81

Reviewer: Opus 5, per the standing rule that an agent-raised PR gets a cross-review from a different model than the one that wrote it. All measurements on CPython 3.14.7 with PYTHONSAFEPATH=1 and PYTHONPATH pinned to my worktree, asserting pcapkit.__file__ resolves inside it before importing anything else. Exit codes read from files, never from summary lines.

Shape: one commit on top of current main 8cfd6ab01 (verified git merge-base --is-ancestor), 6 files, +243/−29.

---

1. The thing I set out to falsify: does declaring real fields shift the wire format?

This was my principal worry. MPTCP previously declared kind/length only under TYPE_CHECKING. Making them real fields adds two octets to the schema's field list, and if anything else was already consuming those two octets, every MPTCP parse would shift by two and break.

Nothing was consuming them, and the comment's reasoning is right. _MPTCP.test is a ForwardMatchField — it peeks and rewinds, consuming nothing — and mptcp_data_selector then builds SchemaField(length=pkt['test']['length'], schema=schema), which reads from the current position, i.e. the option's first octet. So the nested subtype schema does receive the whole option including kind and length, and before this PR its first declared field (test, the subtype/flags octet) was reading the kind octet.

I verified that by measurement rather than by reading. Hand-built, spec-correct ADD_ADDR options (RFC 8684 §3.4), parsed through TCP(io.BytesIO(...)):

option	bytes	main @ 8cfd6ab01	this branch
IPv4, no port	1e08340101020304… (1e083401c0000201)	FieldError: TCP: [OptNo 30] 3 invalid IP version	parses: subtype 3, length 8, addr_id 1, port None
IPv4, with port	1e0a3401c00002011f90	FieldError: … invalid IP version	parses: length 10, port 8080
IPv6, no port	1e14360120010db8…01	FieldError: … invalid IP version	parses: length 20, port None
IPv6, with port	1e16360120010db8…011f90	FieldError: … invalid IP version	parses: length 22, port 8080

The main failure is exactly the predicted misalignment: MPTCPAddAddress.test read the kind octet 0x1e, so version came out as 0x1e & 0x0f == 14, and mptcp_add_address_selector rejects anything that is not 4 or 6.

And it is not only ADD_ADDR. A hand-built MP_PRIO (1e045109 — subtype 5, backup set, addr_id 9, length 4):

main:         RAISED builtins.KeyError: 'length'
this branch:  subtype = <MPTCPOption.MP_PRIO: 5>, length = 4, backup = True, addr_id = 9

All four values exactly as constructed. So this PR repairs MPTCP parsing of real wire bytes, not just construction — the change is strictly a correction of an existing off-by-two, not a wire-format shift. That is a considerably stronger result than the PR title and summary claim, and it is the basis of my requests 1 and 2.

One accuracy correction. The new schema comment says unpacking "silently misread the kind octet as the subtype/flags octet". In both subtypes I measured it does not fail silently — it raises (FieldError for ADD_ADDR, KeyError for MP_PRIO). Silent misreading may well be what happens for subtypes with no conditional field and no validation (MP_FAIL, MP_FASTCLOSE, REMOVE_ADDR), which I did not test; but as written the comment understates ADD_ADDR and MP_PRIO, where the old behaviour was a hard error. Worth a word change, since "silently" is doing load-bearing work in the reader's sense of severity.

2. It did not make UnknownFieldWarning fatal — confirmed

I checked this specifically, since making it fatal would have broken the ~11 _make_mptcp_* call sites that rely on **kwargs forwarding, which pcapkit/protocols/schema/schema.py:95-105 documents as deliberate. schema.py is not in the diff at all. The fix gives the keywords somewhere to land rather than making their absence an error, which is the right shape.

3. Fails-without-the-fix — measured

Reverted pcapkit/protocols/schema/transport/tcp.py to its main content, keeping the PR's tests, then restored from a byte-identical copy and confirmed git status clean.

5 failed, 3 warnings in 0.72s
exit code (from file): 1

All five new tests fail, on exactly the documented cause:

E  AssertionError: unexpected UnknownFieldWarning(s):
   ["'kind' is not a valid field name", "'length' is not a valid field name"]

Your "verified each fails … on the pre-fix code" claim is corroborated.

4. The tests assert packed bytes, as they must

Checked because construction succeeds and only pack() fails, so a construction-only test would pass on broken code. These do not: every case calls schema.pack() and asserts exact octets, for both the port-present and port-absent branches on both address families.

IPv4 no port    1e08340101020304          (8 octets,  length octet 8)
IPv4 with port  1e0a34010102030401bb      (10 octets, port 0x01bb)
IPv6 no port    …                         (20 octets, length octet 20)
IPv6 with port  …                         (22 octets, port 0x01bb)

I checked those byte strings against RFC 8684 §3.4 by hand: 1e kind 30, 08/0a length, 34 = subtype 3 << 4 | version 4, 01 addr_id, then the address, then the port. Correct.

The gap is the other direction (request 1): nothing here constructs a TCP from bytes. test_opt_argument_round_trips_through_the_maker is maker→maker — it passes a previously-parsed opt object back into the maker — not bytes→parse, so despite the name it does not cover the unpack repair.

5. The EXPECTED_FAILURES re-points are right, and the citations are exact

I checked each line number rather than trusting them:

• pcapkit/protocols/schema/transport/tcp.py:650 is exactly subtype: 'Enum_MPTCPOption', inside the if TYPE_CHECKING: block. Correct — and it names the cause rather than the raise site, matching the convention in neighbouring entries.
• pcapkit/protocols/transport/tcp.py:2669 is exactly length=20 if rkey is None else 32,. Correct.
• Fragments are per-class — "'MPTCPCapable' object has no attribute 'subtype'", "'MPTCPAddAddress' object has no attribute 'subtype'", and so on. That is the right call: a shared "no attribute 'subtype'" would have matched all seven cases and pinned none of them, which is precisely what Gap.fragment's docstring warns against. Each entry now pins its own case.
• Re-pointing rather than deleting is correct, since none of the seven round-trips yet.
• tcp-mptcp/MP_JOIN correctly left alone on no attribute '_flags'.
• Issues MPTCP.subtype is declared only under TYPE_CHECKING, so every subtype built via TCP's convenience constructor raises AttributeError #566 and TCP._make_mptcp_capable writes length 20/32 where RFC 8684 gives 12/20, and now packs wrong bytes rather than crashing #567 both exist and are open, with accurate titles.

pytest tests/protocols/test_option_roundtrip_unit.py → exit 0, 6 passed, 358 subtests.

6. #567 is scoped too narrowly — the schema's own predicate is also wrong

This is request 3, and it is the finding I would most like acted on while the context is fresh, because #567 as written will produce a fix that is still broken.

MPTCPCapable.rkey is gated on

rkey: 'int' = ConditionalField(
    UInt64Field(),
    lambda pkt: pkt['length'] != 32,
)

— include the receiver key for any length except 32. Meanwhile the maker writes length = 20 if rkey is None else 32, i.e. it signals "receiver key present" with 32. Those two are in direct contradiction: the value the maker uses to mean "rkey present" is the one value the schema reads as "omit rkey".

Measured on this branch, packing MPTCPCapable directly with skey=0x0102030405060708:

rkey supplied?	declared length	actual packed	result
yes	32 (what the maker writes)	12 octets	1e2001010102030405060708 — receiver key silently dropped, length octet claims 32
yes	20 (RFC 8684 §3.1)	20 octets	1e14010101020304 05060708 deadbeefcafef00d — correct
no	20 (what the maker writes)	20 octets	1e14…0000000000000000 — spurious all-zero receiver key
no	12 (RFC 8684 §3.1)	20 octets	still emits the zero receiver key; the schema cannot express a 12-octet MP_CAPABLE at all

So the last row is the important one: correcting only the maker to 12/20, as #567 describes, leaves the no-rkey case emitting a 20-octet option with a phantom zero key. The != 32 predicate has to change too — presumably to something that distinguishes 12 from 20. RFC 8684 §3.1 gives MP_CAPABLE as kind(1) + length(1) + subtype/version(1) + flags(1) + sender key(8) = 12 octets, plus receiver key(8) = 20.

None of this is #565's business and deferring it was right. I am only asking that #567's description be corrected so the follow-up is complete.

7. Other checks

• Changelog generated, not hand-matched: python util/changelog_md.py --check → exit 0, "in step with docs/source/changelog/1.5.0.rst". (Content gap is request 2; the mechanics are fine.)
• pytest tests/protocols/transport → exit 0, 82 passed, 73 subtests — matching your claimed numbers exactly.
• Full tier pytest tests (no coverage, per the host memory constraint) → exit 0 read from a file, 1265 passed, 17 skipped, 2850 subtests passed in 1133.58s. Your "0 failed" is corroborated, and the count is self-consistent: I measured 1261 on fix(corekit): stop a malformed TCP SACK's exception type depending on sys.modules state (#525) #562's branch, which is main's 1260 plus its own one new test, so main + this PR's five new tests = 1265. Fixtures generated first via examples/generators/make_samples.py (exit 0), since a fresh worktree has none.
• The amend 9e93c7772 → 45d37ad81 touches only tests/protocols/test_option_roundtrip_unit.py (+40/−21); the source diff is unchanged, as intended.
• The seven re-pointed fragments are verified against real behaviour, not just eyeballed. The harness asserts fragment in outcome.detail for every recorded gap, and the roundtrip run passes with 358 subtests — so each of the seven per-class fragments was matched against the detail the current code actually produces.
• Scope justification — I think this is genuinely well done. Naming all ten other _make_mptcp_* helpers the one change also fixes, and separating the newly-exposed defects into MPTCP.subtype is declared only under TYPE_CHECKING, so every subtype built via TCP's convenience constructor raises AttributeError #566 and TCP._make_mptcp_capable writes length 20/32 where RFC 8684 gives 12/20, and now packs wrong bytes rather than crashing #567 rather than folding speculative fixes in here, is the right division. The _make_mptcp_join/_flags exclusion is also correctly characterised as unaffected either way.

Could not verify

• CI. Reached deliberately on local evidence only; I did not wait on GitHub Actions and make no claim about its tally.
• Any interpreter other than CPython 3.14.7.
• Whether unpacking fails silently for MP_FAIL, MP_FASTCLOSE or REMOVE_ADDR on main (§1). I measured ADD_ADDR and MP_PRIO only; both raise. I am not claiming the "silently" wording is wrong for every subtype, only that it is wrong for the two I tested.
• Real MPTCP capture behaviour. All my parse evidence is from hand-built options I checked against RFC 8684 by hand, not from a capture file.
• That the ten other _make_mptcp_* helpers all now construct and pack, which the description says was confirmed by hand for each. I verified ADD_ADDR thoroughly and MP_CAPABLE and MP_PRIO incidentally; I did not walk all ten.

[JarryShaw]

❌ NEEDS CHANGES

Re-review after the force-push, head 138878799 (previously reviewed at 45d37ad81). Cross-model: Opus 5; the PR was authored on Sonnet.

One of my two items is fully addressed. The other is untouched, and it is a one-sentence fix.

Item 1 — parse-direction test: done, and done well. The new TCPMPTCPUnpackUnitTests adds exactly the two cases I measured, driving real octets through TCP(raw, len(raw)):

• test_add_addr_spec_octets_parse_correctly — RFC 8684 §3.4.1 IPv4 ADD_ADDR, asserting kind, length == 8, subtype == ADD_ADDR, version == 4, addr_id == 1, addr == '192.0.2.1' and port is None.
• test_mp_prio_spec_octets_parse_correctly — MP_PRIO, asserting kind, length == 4, subtype == MP_PRIO, addr_id == 5.

Both docstrings name the pre-fix failure precisely, including the 0x1e → version = 14 derivation. I verified the file rather than reading it: 7 passed, exit 0 with the fix; with pcapkit/protocols/schema/transport/tcp.py reverted to main, 7 failed, exit 1 — both new parse tests among them. So the unpack repair is now genuinely pinned. The retitled commit ("repairing pack and parse") is the right call too.

Item 2 — the changelog still covers only the packing direction, unchanged. git diff 45d37ad81 138878799 touches only tests/protocols/transport/test_tcp_mptcp_addaddr_unit.py (+128), so docs/source/changelog/1.5.0.rst is byte-identical to the version I reviewed. It still ends at "...so MPTCP now declares both for real, the same way Option already did (#541)." — entirely about _make_mptcp_* and .pack().

That is the half a user will actually notice. On main, spec-correct MPTCP option bytes do not parse: ADD_ADDR raises FieldError: TCP: [OptNo 30] 3 invalid IP version and MP_PRIO raises KeyError: 'length' — both measured, on both trees. Anyone whose capture failed on 1.5.0b4 will search the changelog for that and find nothing. One or two sentences on the read side and this is done; the mechanics are already fine (util/changelog_md.py --check exits 0, so the generated CHANGELOG.md will follow automatically once the .rst entry is edited).

Everything else remains green at this head: 7 passed / exit 0 on the new file, changelog drift exit 0, and the shape is still one commit on current main 8cfd6ab01. My earlier findings on the EXPECTED_FAILURES re-points (per-class fragments, both cited line numbers exact, MP_CAPABLE naming both #566 and #567) all still stand — that part of the amend was clean and I am not re-raising it.

The #567 scoping note from my previous comment also still stands as a finding on #567 rather than on this PR: MPTCPCapable.rkey's predicate pkt['length'] != 32 contradicts the maker's length=20 if rkey is None else 32, so correcting only the maker to RFC 8684's 12/20 would leave the no-rkey case packing a phantom all-zero receiver key. Nothing for this PR to do.

[JarryShaw]

Addendum on the parse tests — bytes verified, and one item still open

Checked at head 138878799, following a question about whether the new parse tests' octets are internally consistent (a draft of the worker's report self-corrected mid-sentence between 01020304 and c0000201, which are different addresses — 1.2.3.4 and 192.0.2.1).

The committed test uses the correct form, and it is fully self-consistent. The literals are module-level constants:

ADD_ADDR_SPEC_OCTETS = bytes([0x1E, 0x08, 0x34, 0x01, 0xC0, 0x00, 0x02, 0x01])
MP_PRIO_SPEC_OCTETS  = bytes([0x1E, 0x04, 0x51, 0x05])

octet	value	asserted
0x1E	30 = Multipath_TCP	data.kind == Multipath_TCP ✓
0x08	declared length 8 — and the literal is exactly 8 octets ✓	data.length == 8 ✓
0x34	subtype 3 (ADD_ADDR) << 4 | version 4	subtype == ADD_ADDR, version == 4 ✓
0x01	addr_id 1	addr_id == 1 ✓
C0 00 02 01	192.0.2.1	str(data.addr) == '192.0.2.1' ✓
—	no port octets, and 8 ∉ (10, 22)	assertIsNone(data.port) ✓

MP_PRIO likewise: 0x04 declared against exactly 4 octets, 0x51 = subtype 5 (MP_PRIO) with the backup bit (bit 7) set, 0x05 = addr_id 5, matching data.addr_id == 5. Both are 4-octet-aligned so no NOP padding is needed, and build_tcp_segment enforces that with a % 4 guard.

And the technique really is maker-free, which was the defect in the old test_opt_argument_round_trips_through_the_maker. build_tcp_segment passes the raw octets straight through as options=option_octets into Schema_TCP(...) and returns schema.pack(), with offset computed as 5 + len(option_octets) // 4. Grepping the TCPMPTCPUnpackUnitTests class for _make_ returns nothing. So the parse path is exercised with no maker anywhere in it — exactly right.

That closes my item 1 with no reservations.

Item 2 is still open. The changelog entry has not changed. git diff 45d37ad81 138878799 touches only tests/protocols/transport/test_tcp_mptcp_addaddr_unit.py (+128), so docs/source/changelog/1.5.0.rst is byte-identical to the version I first reviewed, and its entry still ends at:

...so MPTCP now declares both for real, the same way Option already did (#541).

Everything in it is about _make_mptcp_* and .pack(). Nothing records that MPTCP options from a capture did not parse at all before this change — which, given the PR is now retitled "repairing pack and parse" and the body names both error strings, is the one place the two have got out of step. One or two sentences on the read side and this is done; util/changelog_md.py --check already exits 0, so the generated CHANGELOG.md will follow automatically from editing the .rst.

Verdict unchanged from my previous comment: ❌ NEEDS CHANGES, for that alone. The code, the tests and the EXPECTED_FAILURES re-points are all good and I am not re-raising any of them.

[JarryShaw]

✅ GOOD TO MERGE

Final re-review, head 9001e4cf1 (reviewed previously at 45d37ad81 and 138878799). Cross-model: Opus 5; the PR was authored on Sonnet.

Both of my items are now closed. The delta from 138878799 is the two changelog files only — git diff 138878799 9001e4cf1 -- pcapkit/ tests/ is empty — so my item 1 verification carries over untouched, and the source diff has now been stable since 9e93c7772 across three rounds of review.

On the changelog wording, which is all you asked me to re-read: it does the job, and better than I asked for. The two things a user needs are both there.

The searchable strings are verbatim and exact. FieldError: TCP: [OptNo 30] 3 invalid IP version and KeyError: 'length' are character-for-character what I measured on main for ADD_ADDR and MP_PRIO respectively, so someone who hit either and pastes it into a search will land on this entry. That was the whole point of the request.

And the distinction reads clearly to an outsider — this sentence is the one that earns its place:

an off-by-two in field alignment rather than a wire-format change -- a correct sender's octets were always right, only this library's reading of them was shifted

That answers the question a user actually has after a capture fails to parse, which is "is my capture corrupt?" — no, and the entry says so in plain terms without requiring the reader to know what _MPTCP.test's ForwardMatchField does. It also correctly frames the mechanism ("a Multipath TCP subtype schema's own leading field read the kind octet itself rather than the octet meant for it") at a level someone who has not followed this thread can follow.

One judgement call I think it got right rather than wrong: it says "Spec-correct ADD_ADDR and MP_PRIO options failed to parse" — naming only the two subtypes actually measured, while the preceding sentence describes the misalignment generally for "a Multipath TCP subtype schema". That is the honest scoping. I only measured those two myself, and I noted earlier that "silently misread" was wrong for both because both raise loudly; claiming all eight subtypes had been verified would have overstated it. The generality is conveyed by the mechanism sentence and the specifics stay inside what was checked.

CHANGELOG.md is genuinely regenerated rather than hand-matched: I ran the repository's own gate, python util/changelog_md.py --check → exit 0, "in step with docs/source/changelog/1.5.0.rst". And the CHANGELOG.md diff is confined to the single entry line, carrying the same prose transformed into the generator's markdown rendering — no other entry disturbed, which matters given how many changelog entries were in flight tonight.

Nothing further from me. For the record, the full set of things I verified across the three rounds and am not re-raising: the wire-format risk (this is a correction of an existing off-by-two, not a shift — spec-correct ADD_ADDR and MP_PRIO bytes do not parse on main and do on this branch); schema.py untouched so UnknownFieldWarning was not made fatal; all seven EXPECTED_FAILURES re-points with per-class fragments and both cited line numbers exact, MP_CAPABLE naming both #566 and #567; packed-byte assertions on all four ADD_ADDR combinations; maker-free, byte-consistent parse tests; 7 of 7 tests failing with the fix reverted; transport tier 82→90 passed; roundtrip 358 subtests; and the full tier at 1265 passed / 17 skipped / 2850 subtests, exit 0.

The MPTCPCapable.rkey finding from my earlier comments has been carried into #567, which is where it belongs.