tests/protocols/transport/test_tcp_udp_unit.py assigns a plain Python set to TCP._flags on a bare TCP.__new__(TCP). Production assigns an aenum.IntFlag. The tests therefore execute every branch of the MP_JOIN dispatchers while bypassing both the attribute's type and the ordering that governs when it exists — which is how #587 hid behind 100% statement and branch coverage on both TCP modules.
The sites
tests/protocols/transport/test_tcp_udp_unit.py:490 proto._flags = {Flags.SYN}
tests/protocols/transport/test_tcp_udp_unit.py:503 proto._flags = {Flags.SYN, Flags.ACK}
tests/protocols/transport/test_tcp_udp_unit.py:516 proto._flags = {Flags.ACK}
tests/protocols/transport/test_tcp_udp_unit.py:828 proto._flags = {Flags.SYN}
tests/protocols/transport/test_tcp_udp_unit.py:834 proto._flags = {Flags.SYN, Flags.ACK}
tests/protocols/transport/test_tcp_udp_unit.py:840 proto._flags = {Flags.ACK}
plus further _flags references around :845, :937, :961, :977 and :990.
A Python set supports the in membership tests the dispatchers use, so every branch runs and every line is covered. But it is not what TCP.make or TCP.read produce, and assigning it directly means the test never exercises whether the attribute exists at the moment the option makers read it — which was the entire substance of #587.
Why this is worth its own issue rather than a note
Coverage of both TCP modules was already 100% statement and branch on main while the public MP_JOIN construction path was broken for every caller. That is the fact worth keeping: a coverage number cannot distinguish "this branch was exercised under production conditions" from "this branch was exercised with a hand-placed attribute of the wrong type on an object that never ran __init__".
It also hid a second defect that only surfaced once #597's hoist made the no-SYN-no-ACK branch reachable: cast('Enum_Flags', 0) is a runtime no-op, so _flags stayed a plain int and the first membership test raised TypeError rather than the documented ProtocolError. A test that had used the real type would have caught it.
Suggested direction
Construct through the public path — TCP.make(...) or a parsed instance — so _flags is whatever production actually assigns, and let the dispatchers be reached the way a caller reaches them. Where a specific flag combination is needed, set it through the syn/ack parameters rather than by writing the private attribute.
The change is expected to be behaviour-identical, but it has its own blast radius across a large test file, which is why #597 disclosed it rather than folding it in.
A related prose cleanup in the same area, now actionable
mptcp_dss_ack_selector's docstring at pcapkit/protocols/schema/transport/tcp.py:268-278 says a corrected field-length lambda "would not have worked" and that fixing it "belongs to pcapkit.corekit.fields.numbers". That fix landed in #598, so the prose is now stale. The same paragraph appears in tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py:82-88.
This was not stale when #597 was decided — #598 merged at 23:40:47Z, six minutes after #597 was raised at 23:34:36Z, so correcting it then would have made that PR assert something untrue of its own base. It is prose only; no assertion depends on it, and #585's SwitchField workaround itself correctly stays, because its NoValueField() branch handles the field being absent from the wire, which a callable-length NumberField cannot express.
Found by the #587 worker; both items disclosed in #597's body.
tests/protocols/transport/test_tcp_udp_unit.pyassigns a plain PythonsettoTCP._flagson a bareTCP.__new__(TCP). Production assigns anaenum.IntFlag. The tests therefore execute every branch of the MP_JOIN dispatchers while bypassing both the attribute's type and the ordering that governs when it exists — which is how #587 hid behind 100% statement and branch coverage on both TCP modules.The sites
plus further
_flagsreferences around:845,:937,:961,:977and:990.A Python
setsupports theinmembership tests the dispatchers use, so every branch runs and every line is covered. But it is not whatTCP.makeorTCP.readproduce, and assigning it directly means the test never exercises whether the attribute exists at the moment the option makers read it — which was the entire substance of #587.Why this is worth its own issue rather than a note
Coverage of both TCP modules was already 100% statement and branch on
mainwhile the public MP_JOIN construction path was broken for every caller. That is the fact worth keeping: a coverage number cannot distinguish "this branch was exercised under production conditions" from "this branch was exercised with a hand-placed attribute of the wrong type on an object that never ran__init__".It also hid a second defect that only surfaced once #597's hoist made the no-SYN-no-ACK branch reachable:
cast('Enum_Flags', 0)is a runtime no-op, so_flagsstayed a plainintand the first membership test raisedTypeErrorrather than the documentedProtocolError. A test that had used the real type would have caught it.Suggested direction
Construct through the public path —
TCP.make(...)or a parsed instance — so_flagsis whatever production actually assigns, and let the dispatchers be reached the way a caller reaches them. Where a specific flag combination is needed, set it through thesyn/ackparameters rather than by writing the private attribute.The change is expected to be behaviour-identical, but it has its own blast radius across a large test file, which is why #597 disclosed it rather than folding it in.
A related prose cleanup in the same area, now actionable
mptcp_dss_ack_selector's docstring atpcapkit/protocols/schema/transport/tcp.py:268-278says a corrected field-length lambda "would not have worked" and that fixing it "belongs topcapkit.corekit.fields.numbers". That fix landed in #598, so the prose is now stale. The same paragraph appears intests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py:82-88.This was not stale when #597 was decided — #598 merged at 23:40:47Z, six minutes after #597 was raised at 23:34:36Z, so correcting it then would have made that PR assert something untrue of its own base. It is prose only; no assertion depends on it, and #585's
SwitchFieldworkaround itself correctly stays, because itsNoValueField()branch handles the field being absent from the wire, which a callable-lengthNumberFieldcannot express.Found by the #587 worker; both items disclosed in #597's body.