fix(ipv4): terminate the SEC bitmap, reject its terminator bit, and narrow SID to 16 bits

[JarryShaw]

Closes #534
Closes #537

Three defects in the IPv4 SEC option, one in the SID option schema, and one test-coverage gap that #536 left behind. They are in one PR because they are one file cluster — pcapkit/protocols/internet/ipv4.py, its schema, and the IPv4 option tests.

Every measurement below was re-derived on this branch's base, not taken from the issues.

#537(a) — the writer emitted an option its own reader rejected

RFC 1108 §2.2 makes bit 0 of each protection-authority octet the field termination indicator: 0 means another octet follows, 1 means this is the last. _read_opt_sec enforces it. _make_opt_sec built the bitmap purely from authority bit positions and never set it.

before:  authorities=[GENSER, NSA] -> data=b'\x90'   # 0x90 & 0x01 == 0, own reader warns
after:   authorities=[GENSER, NSA] -> data=b'\x91'   # 1001 0001

Fix: data_list[-1] = b'1' after the bitmap is built. Intermediate octets keep the 0 they were initialised with, which is exactly what "another octet follows" means, so one assignment is the whole indicator.

Fails without the fix — reverted data_list[-1] = b'1', kept everything else:

test_ipv4_make_opt_sec_sets_the_field_termination_indicator
E  AssertionError: b'\x90' != b'\x91'
EXIT CODE 1

#537(b) — a bare IndexError escaped

int_len = math.ceil(max_auth / 8) sized the bitmap from the highest bit index rather than the bit count. With GENSER (value 0) the only authority, int_len was 0, data_list was empty, and data_list[0] = b'1' raised IndexError: list assignment index out of range — not an in-library exception, and out of a _make_opt_* helper. The same off-by-one under-sized by a whole octet at every exact multiple of eight; no shipped enum member has value 8, so that half was latent rather than reachable.

Fix: math.ceil((max_auth + 1) / 8).

[GENSER]                 -> b'\x81'      (was IndexError)
[ProtectionAuthority(6)] -> b'\x03'
[ProtectionAuthority(8)] -> b'\x00\x81'  (2 octets; ceil(8/8) said 1)
[]                       -> b''          (bare 3-octet option, no bitmap)

Fails without the fix — restored math.ceil(max_auth / 8):

test_ipv4_make_opt_sec_sizes_the_bitmap_from_the_bit_count
E  IndexError: list assignment index out of range
EXIT CODE 1

#537(c) — index 7 was both an enum member and the terminator bit

Enum_ProtectionAuthority member 7 is named Field_Termination_Indicator — structure, not an authority — yet the writer accepted it, producing data=b'\x01': an option reading as validly terminated while encoding zero authorities.

The writer and reader disagreed, and this records which one won. _read_opt_sec loops range(7) per octet and maps octet base bit bit to authority base * 8 + bit, so the numbering it produces skips 7, 15 and 23 — those positions are termination bits and nothing else. The reader is right and the writer was wrong, so the rejection is on the write side and range(7) is deliberately left alone. Widening it would make the reader report a terminator as an authority.

Fix: reject auth % 8 == 7 with ProtocolError, and auth < 0 too — a negative index would have written to the terminator through Python's negative indexing rather than raising, which is silent corruption and the one way the IndexError could still have been reached after (b).

Rejecting the whole congruence class rather than only the named 7 follows from the reader's numbering: 15 and 23 are termination bits for the same reason and are just as undeliverable, they simply have no name in the enumeration yet.

IPv4: [OptNo 130] invalid protection authority: Field_Termination_Indicator is a field termination indicator, not an authority
IPv4: [OptNo 130] invalid protection authority: -1

The message uses getattr(auth, 'name', auth) rather than Enum_ProtectionAuthority.get(auth), because the latter runs _missing_ for an unnamed index and extends the enumeration as a side effect — an error path should not mutate a registry.

Fails without the fix — removed the validation loop:

test_ipv4_make_opt_sec_rejects_a_termination_bit_as_an_authority
E  AssertionError: ProtocolError not raised
EXIT CODE 1

#534 — SIDOption.sid was 32 bits where RFC 791 gives 16

RFC 791 §3.1 gives the Stream ID option as four octets: type, length, and a two-octet identifier. _make_opt_sid already wrote length=4; only the schema field disagreed, so it over-read a well-formed option by two octets on the way in and over-wrote it by two on the way out. Six not being a multiple of four, the option area then reached the 32-bit padding branch and picked up a NOP and an EOOL.

88040037 off the wire	before	after
parse warning	packet length < 0: -2	none
re-emitted option	880400000037	88040037
option schemas	SIDOption, NOPOption, EOOLOption	SIDOption
rebuilt datagram	8804000000370100, 28 octets, ihl 7	88040037, 24 octets, ihl 6

Fix: UInt16Field. A parsed datagram now rebuilds byte-identically to the octets it was read from.

Fails without the fix — restored UInt32Field:

test_ipv4_sid_option_is_four_octets_wide_on_the_wire
E  AssertionError: 'packet length < 0: -2' unexpectedly found in ['packet length < 0: -2']
EXIT CODE 1

Coverage gap #536 left behind

#536 fixed Schema.pack to check isinstance(data, ProtocolBase) instead of Protocol. Its reviewer showed by falsification that a wrong fix passes all four of #536's new tests, because every shipped test's "real protocol" payload is only Raw(...) or NoPayload().

Reproduced. Applying

elif isinstance(data, (Protocol, Raw, NoPayload)):

— special-casing exactly the classes the tests use — gives:

4 passed        EXIT CODE 0

while the general defect survives for the other 40 ProtocolBase subclasses. Measured by a __subclasses__() walk: Protocol has 0 descendants, ProtocolBase has 43, and 40 are outside {Protocol, Raw, NoPayload}.

New test: test_schema_pack_packs_a_second_real_protocol_as_payload — a real UDP nested in IPv4, plus TCP and IPv4 as second and third payloads, asserting each is a ProtocolBase and none of the three classes a special-case list would name. It also asserts the uncovered-descendant count, so the reason a class list cannot be the fix is recorded as a number rather than a remark.

Fails without the fix (fake fix applied):

E  pcapkit.utilities.exceptions.ProtocolUnbound: unsupported type <class 'pcapkit.protocols.transport.udp.UDP'>
   pcapkit/protocols/schema/schema.py:700
EXIT CODE 1

Passes with the real fix restored (schema.py byte-identical to main): EXIT CODE 0. pcapkit/protocols/schema/schema.py is not modified by this PR — part 3 is test-only.

The OptNo 130 warning is gone from the regenerated fixture

examples/captures/options-ipv4.pcap regenerated with python examples/generators/make_samples.py and re-extracted:

before:  IPv4: [OptNo 130] invalid format: field termination indicator not set
         EOF reached
after:   EOF reached

The SEC option now reads back with no ProtocolWarning at all, and its flags round-trip to (GENSER, NSA). The captures are gitignored, so nothing generated is committed.

The generator's workaround comment at examples/generators/options.py:469 is updated rather than deleted: the pair of authorities is kept, because two bits set say more than one, but it is now a coverage choice and no longer routes around a defect. That the library no longer needs it is proved by the new test passing [GENSER] alone.

EXPECTED_FAILURES reconciliation

Before: 55 entries. After: 55 entries — unchanged. Imported rather than grepped, since it uses ** unpacking.

ipv4-option keys, before and after: ['ipv4-option/QS', 'ipv4-option/TS']
ipv4-option/SEC: status='OK'
ipv4-option/SID: status='OK'

No entry became stale and none started passing, so there was nothing to reconcile. test_round_trip_is_identity_or_a_recorded_gap and test_expected_failures_name_real_cases were each run in isolation and checked by exit code (0), not by the summary line — pytest-subtests is not installed and pytest 9.1.1 prints a failing subtest's parent as PASSED.

Test changes that are not new tests

test_a_parsed_sid_option_re_emits_two_octets_too_wide is removed from tests/protocols/test_option_roundtrip_unit.py, which is what #534 asks for: it existed only because the defect could not be expressed as an EXPECTED_FAILURES entry, its cycle closing either way. Its coverage is not lost — it is inverted and relocated to test_ipv4_sid_option_is_four_octets_wide_on_the_wire, which starts from the same wire octets and asserts more: byte-identical rebuild, no padding, no warning. The module docstring and the EXPECTED_FAILURES comment are updated to point at it, and the now-unused warnings import is dropped.

Four existing assertions changed because they encoded the old behaviour:

• Two passed Field_Termination_Indicator as an authority — i.e. they were unknowingly relying on defect (c) to terminate the option. One now passes [GENSER] alone and the expected octets are unchanged (b'\x81'); the other uses (GENSER, NSA), which is also the more faithful data model since _read_opt_sec never puts the terminator in flags.
• Three option-area lengths shrank because SID is no longer over-wide: 32 -> 28 with ihl 7 -> 6, 12 -> 8, 8 -> 4, 20 -> 24 (the last grew because a SEC option was added, see below).

#506's EOOL-padding coverage was at risk and is preserved deliberately. SID's old 6-octet width was what drove the 32-bit alignment branch in two places; with SID 4-aligned it no longer reaches that branch at all, which would have silently dropped the coverage. So both paths get an explicit trigger instead:

• the list branch gets a new case built on a 5-octet SEC option (two NOPs and an EOOL);
• the data-model branch gets a SEC option added to its OrderedMultiDict, which also restores the for _ in range(pad_len - 1) multi-NOP arm — that line went uncovered when SID stopped being a 6 % 4 == 2 trigger.

Verification

Repo venv, PYTHONSAFEPATH=1, and pcapkit.__file__ asserted to be inside this worktree on every measurement, so no editable install in site-packages could shadow the tree under test.

• Full suite: EXIT CODE 0 — 1181 passed, 17 skipped, 2661 subtests.
• tests/protocols/: EXIT CODE 0 — 488 passed, 1381 subtests.
• tests/test_docstring_contract.py: EXIT CODE 0. (It failed on pcapkit/vendor/ipx/packet.py before the rebase; that was pre-existing on main — verified against a pristine tree — and docs(hopopt): cite RFC 8200 section 4.2 for the Opt Data Len sentence (#530) #538 has since fixed it.)

Coverage of the two changed library files, main versus this branch:

	stmts	miss	branch	brpart	cover
main internet/ipv4.py	465	0	172	1	99%
this branch	471	0	178	1	99%
schema/internet/ipv4.py	172	0	24	0	100%
this branch	172	0	24	0	100%

+6 statements and +6 branches, all covered, with no new gap — the single remaining partial (322->324) is pre-existing.

Found but deliberately NOT fixed

• _read_opt_sec's for bit in range(7) is left as it is. It is the correct half of IPv4 SEC option: the writer emits an option its own reader rejects, and IndexErrors on a single authority of value 0 #537(c): bit 0 of each octet is structure, so excluding it is right, and the writer was changed to agree. Widening it to range(8) would make the reader report a terminator as an authority.
• The reader still only warns on a missing termination indicator rather than raising, and still warns rather than raising on remaining data and on an unknown authority. Loosening or tightening that is a behaviour change for parsing real-world captures and is out of scope here; the writer no longer produces any of those warnings.
• ProtectionAuthority._missing_ extends the enumeration on any non-negative int lookup, so ProtectionAuthority(9999) permanently adds Unassigned_9999 to a process-wide enum. The new code avoids triggering it, but the underlying registry-mutation-on-read behaviour is untouched — it is shared by every const enum in the tree and is a separate design question.
• _read_opt_sec's base < schema.length - 4 guard for the remaining data warning reads as an off-by-one risk for a 1-octet bitmap, but it is correct as written (length - 4 is the last octet's index) and no change was needed.
• SIDOption has no upper-bound validation on sid. A caller passing sid=0x10000 now overflows a 16-bit field rather than being rejected with a clear message. Pre-existing for every other narrow field in the schema and not specific to this change.

Not done

Not merged, not tagged, not released.

[JarryShaw]

✅ GOOD TO MERGE — independently reproduced all five fails-without proofs at exit code 1 (SEC terminator bit, SEC bitmap sizing IndexError, SEC index-7 rejection, SID width, and the #536 coverage-gap fake-fix), confirmed the RFC 1108/RFC 791 field-width claims against the source directly, and confirmed by direct removal that narrowing SID really did require the two new SEC-based coverage triggers to keep ipv4.py's multi-NOP padding arm covered on both branches.

[JarryShaw]

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

Head sha reviewed: ff0ea997be1eb6086ec05269dfbc346dbf25a02d.

RFC claims — verified against the source directly, not taken from the PR

Read _read_opt_sec (pcapkit/protocols/internet/ipv4.py:709-763) myself: the loop is for base, byte in enumerate(schema.data): for bit in range(7): authority = Enum_ProtectionAuthority.get(base * 8 + bit), checking byte & (0x80 >> bit) — i.e. bits 7..1 (MSB down) per octet, explicitly skipping bit 0 (LSB). The terminator check is schema.data[-1] & 0x01 == 0. This independently confirms the PR's claim: the reader already treats bit 0 of every octet as structure (the termination indicator), and its indexing (base*8 + bit for bit in 0..6) skips positions 7, 15, 23 exactly as claimed — RFC 1108 section 2.2's "seven authority bits then a terminator" layout, read straight from the code rather than trusted from the docstring.

Falsification — all five fails-without proofs independently reproduced, exit codes read from a file

Working in my own worktree at this head, I made one surgical revert at a time, ran the specific new test, confirmed the failure, then reverted my edit and confirmed a clean diff before the next one:

1. Termination bit (removed data_list[-1] = b'1'): test_ipv4_make_opt_sec_sets_the_field_termination_indicator failed with AssertionError: b'\x90' != b'\x91', exit code 1.
2. Bitmap sizing (restored math.ceil(max_auth / 8)): test_ipv4_make_opt_sec_sizes_the_bitmap_from_the_bit_count failed with IndexError: list assignment index out of range, exit code 1.
3. Index-7 rejection (removed the validation loop): test_ipv4_make_opt_sec_rejects_a_termination_bit_as_an_authority failed with AssertionError: ProtocolError not raised, exit code 1.
4. SID width (restored UInt32Field): test_ipv4_sid_option_is_four_octets_wide_on_the_wire failed with 'packet length < 0: -2' unexpectedly found, exit code 1.
5. Coverage-gap fake fix (special-casing Protocol, Raw, NoPayload in pcapkit/protocols/schema/schema.py, exactly as fix(protocols): let from_data rebuild a parsed packet #536's reviewer used): test_schema_pack_packs_a_second_real_protocol_as_payload failed with ProtocolUnbound: unsupported type <class 'pcapkit.protocols.transport.udp.UDP'>, exit code 1 -- while the same fake fix passes all four of fix(protocols): let from_data rebuild a parsed packet #536's original tests (test_ipv4_from_data_rebuilds_a_parsed_datagram, test_ipv4_make_accepts_a_protocol_instance_as_payload, test_schema_pack_packs_a_protocol_base_payload_on_its_own, test_ipv4_make_options_pads_with_an_eool_option_not_its_wire_code): 4 passed, exit code 0, matching the PR's claim precisely.

All five match the PR's claimed evidence exactly.

Re-derived: 43 ProtocolBase descendants, 40 outside the three special-cased classes

Walked __subclasses__() recursively after importing every submodule under pcapkit.protocols: 43 total ProtocolBase descendants, 0 Protocol descendants, and 40 outside {Protocol, Raw, NoPayload} -- matches the PR's numbers exactly.

Call 3 (the coordinator's flagged subtle claim) -- independently settled, and it holds

The claim: narrowing SID from 6 to 4 octets removes the only existing trigger for the multi-NOP padding arm (for _ in range(pad_len - 1): options_list.append(pad_opt)) on both branches of _make_ipv4_options, so the PR adds two new SEC-based triggers (a 5-octet SEC option, one for the list branch, one for the data-model/OrderedMultiDict branch at ipv4.py:1309) to keep it covered.

I verified this by direct removal rather than by reasoning about it: temporarily changed both new triggers so the constructed SEC option became 4 octets / 32-bit-aligned instead of 5 (so neither reaches the padding branch at all), then re-ran coverage run --include=".../ipv4.py" -m pytest tests/protocols/internet/test_ipv4_unit.py.

Baseline (unmodified head): 471 stmts, 0 miss, 178 branch, 1 brpart, 99%, missing only the pre-existing 322->324.

With both new triggers neutralized: 471 stmts, 3 miss, 178 branch, 4 brpart, 99%, now also missing line 1309 -- exactly the data-model-branch multi-NOP line the coordinator asked about -- confirming Call 3 is correct: this line does go uncovered without the new SEC trigger, meaning the narrowed SID genuinely was the sole prior trigger and the PR's fix is a real, necessary catch rather than incidental churn. (Two other lines, 1582 and 1898 -- unrelated ProtocolError raises in _make_opt_ts/_make_opt_qs -- also showed as missed in this run, but that is a side effect of my edit causing an AssertionError partway through the same test method before it reached those later, unrelated assertions; not a Call 3 finding.) All edits reverted; the worktree diff against the head was empty afterward.

Call 4 -- the four changed pre-existing assertions, checked against the actual diff hunks

Read each hunk directly rather than accepting the PR's categorization:

• test_ipv4_option_constructors_cover_common_and_error_branches: authorities list dropped Field_Termination_Indicator, expected bytes unchanged (b'\x81') -- genuinely a defect-(c)-reliance fix, since _make_opt_sec now sets the terminator bit itself.
• test_ipv4_option_constructors_cover_data_model_and_mapping_paths (the SEC-in-option_map schema): flags swapped Field_Termination_Indicator for NSA -- same category, correctly reasoned (the terminator is never in _read_opt_sec's flags, so naming a real authority is the more faithful data model).
• Two length assertions (total_length: 12 to 8 and schema_total: 8 to 4, len(schema_options): 3 to 1) are pure consequences of SID narrowing (6 to 4 octets), unrelated to the terminator-bit fix.
• The mapped_total: 20 to 24 assertion is the one the PR is explicit about growing rather than shrinking, because a new SEC option was deliberately added to that same option_map for the Call-3 coverage restoration -- confirmed by reading the hunk, which does add a new SEC entry that wasn't there before.

All four check out as claimed; none was changed merely to make a red test green for an unrelated reason.

The generator diff -- not a bug-hiding edit

examples/generators/options.py's only change is to the comment above the SEC authorities override -- the actual generator input (GENSER plus NSA) is byte-for-byte unchanged from main. This is the sharpest falsification available on this PR (a generator edited to avoid a failing input would be indistinguishable from a real fix in the fixture alone), and it does not apply here: nothing about the test input changed, only the prose explaining why it's still worth keeping at two authorities.

The most-checkable outcome -- confirmed, after I initially mismeasured it

Regenerating examples/captures/options-ipv4.pcap and re-extracting: the "OptNo 130 ... field termination indicator not set" warning is gone, leaving only the benign "EOF reached". Worth recording exactly how I got this right, because I got it wrong first: running python examples/generators/make_samples.py with PYTHONSAFEPATH=1 and no explicit PYTHONPATH resolves import pcapkit to the main checkout (the editable install target), not this worktree -- PYTHONSAFEPATH disables the cwd/script-dir prepend that would otherwise have masked the editable install, and unlike a -c invocation with an explicit sys.path.insert, a plain script run has nothing else pointing at the worktree. My first attempt reproduced the warning because it was silently testing main, not this PR. Re-run with PYTHONPATH set explicitly to the worktree root (and pcapkit.__file__ printed and asserted before touching anything else) resolved correctly, and the warning is genuinely gone on this PR's actual code. Flagging this as a process note for future reviews in this programme: PYTHONSAFEPATH=1 on a plain script invocation (not just -c) can still fall through to the editable install unless PYTHONPATH is also set explicitly.

EXPECTED_FAILURES -- confirmed unchanged, with a correction to my own initial read

Imported (not grepped): 55 entries, unchanged from main. The two surviving ipv4-option keys are ipv4-option/QS and ipv4-option/TS, confirmed by string-matching the keys directly (my first attempt indexed the first character of each key assuming tuple keys; they are plain strings -- corrected and re-verified). tests/protocols/test_option_roundtrip_unit.py (6 tests, 358 subtests) passes clean at exit code 0, confirming SEC and SID round-trip as the 'OK' status the PR describes.

Coverage -- independently measured, matches exactly

coverage run over tests/protocols/internet/test_ipv4_unit.py: internet/ipv4.py 471 stmts, 0 miss, 178 branch, 1 brpart, 99% (missing 322->324, pre-existing); schema/internet/ipv4.py 172 stmts, 0 miss, 24 branch, 0 brpart, 100%. Both match the PR's table exactly.

CI status

Not run -- GitHub Actions has been backed up throughout this review session (queued/pending with no declared incident); verdict is on local evidence only, per standing instruction.

What remains unverified

• The full CI-equivalent suite was still running in the background at review time (roughly 60% through after about 10 minutes, sharing the host with other concurrent test runs); I did not wait for it to complete before posting this verdict, since every individual claim it would aggregate has already been independently reproduced above. Will post the exact final numbers as a follow-up comment once it finishes if they differ from the PR's claimed 1181 passed, 17 skipped, 2661 subtests.
• tests/protocols/ in isolation (the PR's second reported figure, 488 passed, 1381 subtests) was not separately re-run.

[JarryShaw]

Follow-up on the full-suite number promised above: my own run of
pytest tests --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'
(the same "unit tier" selection used elsewhere in this review) finished at
EXIT=0, 1043 passed, 5 skipped, 2528 subtests passed in ~15 minutes.

This is narrower than the PR's own claimed "Full suite: 1181 passed, 17 skipped, 2661 subtests" --
that figure appears to be from a broader invocation (likely plain pytest tests including
integration/runtime/regression tests, which my selection explicitly excludes), not a discrepancy in
the underlying fix. My run is a subset check confirming no failures in the unit tier; I did not
separately re-run the wider selection the PR's own number reflects. Either way: exit 0, zero
failures, consistent with everything else verified above.