fix(hip): register R1CounterParameter under HIPv1's code 128 too

[JarryShaw]

Please follow the guide below

• You will be asked some questions, please read them carefully and answer honestly

• Put an x into all the boxes [ ] relevant to your pull request (like that [x])

• Use Preview tab to see how your pull request will actually look like

• Searched for similar pull requests

• Followed the coding style (make pylint, make mypy, make isort) — scoped to touched files/lines; mypy clean, no new pylint/isort findings on my diff

• make test passes, and a test case covers the change — not run in full per instruction; see test counts below

• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A, changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• fix — corrects a defect

---

Description of your pull request and other information

Fixes #690: HIPv1's R1_Counter (128) parsed as UnassignedParameter — the schema registry had
only code=129 registered, even though method dispatch and the maker already handled both codes.
Registers R1CounterParameter for both via EnumSchema's existing code=[...] form.

test file	passed	subtests
test_hip_unit.py	34	168
test_option_roundtrip_unit.py	6	360

Also corrects two prose sites this fix falsifies elsewhere: test_hip_r1_counter_width_unit.py's
"code 128 not exercised" docstring, and options.py's "three remain" / enumeration-member-name
comment (now two — HOST_ID, HIP_TRANSFORM — 47 OK of 49). Rebased three times as main moved
during review; see PR comments for the cross-review trail and falsification evidence.

[JarryShaw]

❌ NEEDS CHANGES — cross-review on Opus of Sonnet-authored work at head 552ff9efa: the one-line registry fix is correct and RFC-backed, but the new test hardcodes the pre-#696 12-octet R1_COUNTER encoding, so the (textually clean) merge of #721 × #696 fails at test_hip_unit.py:2356 — drop or derive that byte-literal, and correct the "does not collide with #696" claim in the PR body.

[JarryShaw]

Cross-review detail — Opus, head 552ff9efa, base 8af2cfda7

pcapkit.__file__ for every run below: /local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a9ee2c26b7c03ccaf/pcapkit/__init__.py (asserted in-process, PYTHONSAFEPATH=1, CPython 3.14.7). Tree restored from explicit cp backups; sha256sum matches and git status --porcelain is empty.

Tests, as shipped: test_hip_unit.py 34 passed / 168 subtests; test_option_roundtrip_unit.py 6 passed / 360 subtests; together 40 passed / 528 subtests.

1. Are 128 and 129 distinct? ✅ deliberate version split, not an accident

const/hip/parameter.py:25,28 — R1_Counter = 128, R1_COUNTER = 129. Imported, not grepped: is → False, == → False, both present in __members__. RFC 7401 says why, twice:

The type code for the R1_COUNTER has been changed from 128 to 129 to reflect that it is now considered a Critical parameter and must be echoed when present in R1.

For clarity, the name for the value 128 has been changed from "R1_COUNTER" to "R1_Counter (v1 only)".

IANA agrees: 128 = R1_Counter (v1 only) [RFC 5201], 129 = R1_COUNTER [RFC 7401]. The docstring's C-bit reading is right — 128 = 0b10000000 / 129 = 0b10000001, and the reader's own critical=bool(schema.type & 0b1) is that bit.

2. Does code=[...] register every element? ✅ verified, 11 prior users

schema/schema.py:1123-1129: codes = code if isinstance(code, collections.abc.Iterable) else (code,), then for _code in codes: … cls.__enum__[_code] = (cls). No element is dropped. Measured on this head:

R1_Counter (128) -> R1CounterParameter
R1_COUNTER (129) -> R1CounterParameter
__default__      = Parameter.<lambda>   # UnassignedParameter

PadOption/RPLOption precedent confirmed (ipv6_opts.py:375,663, hopopt.py:375,658, mh.py:622), plus ExperimentalExtension, BitRateAttribute, CustomOption, CustomBlock — 11 classes before this one. No RegistryWarning from #695 fires: nothing claimed 128 before, and _EnumRegistry.__missing__ never inserts a miss.

3. Is the asymmetry real? ✅ both halves, one correction

• internet/hip.py:383-384 on main has both entries pointing at 'r1_counter'. One correction: they are two hand-written literals in a collections.defaultdict, not a name-normalisation rule — the PR body's "method-name dispatch" phrasing implies a mechanism that does not exist.
• _make_param_r1_counter (internet/hip.py:3011) already returns Schema_R1CounterParameter(type=code, len=12, counter=counter) by direct instantiation, with its own code == R1_Counter and version != 1 guard. Construction side was already complete; the fix is not partial.
• schema/internet/hip.py:1525 OptionField(…, registry=Parameter.registry) → corekit/fields/collections.py:442 schema = self._registry[code] → __missing__ → UnassignedParameter. Sole gap, correctly located. No data-side registration exists or was needed.

Falsification, reproduced. git show 8af2cfda7:…/schema/internet/hip.py swapped in, new test run:

AssertionError: <class '…UnassignedParameter'> is not <class '…R1CounterParameter'>
tests/protocols/internet/test_hip_unit.py:2346

and end-to-end, on the exact bytes the test builds:

AttributeError: 'UnassignedParameter' object has no attribute 'counter'
  at internet/hip.py:887, counter=schema.counter

4. Can one class serve both codes? ✅ yes — but this class is not that shape yet

RFC 5201 §5.2.3 and RFC 7401 §5.2.3 are byte-identical: Type(16) | Length(16) | Reserved, 4 bytes | R1 generation counter, 8 bytes, Length 12 in both; IANA lists Length 12 for 128 and 129 alike. Only the Type value and the criticality differ. So registering one class for both codes is correct, and there is no RFC-width objection to this PR.

What is wrong is that the class this PR registers under both codes declares counter: 'int' = UInt32Field() — 4 octets, not 8. The new docstring asserts the RFCs' "identical 4 + 8 layout" six lines above a field list that implements 4 + 4. Measured on this head:

code-128 record bytes: 0080000c00000000aabbccdd  (12 octets)
declared len field = 12  ->  RFC record total = 4 + 12 = 16

And the test hardcodes that 12-octet form at test_hip_unit.py:2356. git merge-tree --write-tree pr/721 pr/696 merges with no textual conflict (exit 0, tree 62094a2e) — so the "no overlap" claim is right textually and wrong behaviourally. Running the new test against that real merged tree:

pcapkit.__file__ = /tmp/xreview721/merged/pcapkit/__init__.py
FAIL: test_hip_r1_counter_code_128_resolves_to_its_own_schema
  File ".../test_hip_unit.py", line 2356
    self.assertEqual(r1_one, bytes.fromhex('0080000c00000000aabbccdd'))
AssertionError: b'\x00\x80\x00\x0c\x00\x00\x00\x00\x00\x00\x00\x00\xaa\xbb\xcc\xdd'
            != b'\x00\x80\x00\x0c\x00\x00\x00\x00\xaa\xbb\xcc\xdd'

Correcting that single literal to 0080000c0000000000000000aabbccdd makes it pass on the merged tree — the breakage is exactly one line, and nothing else in the test depends on the narrow counter.

Required: drop the byte-literal (the later copy.counter == 0xaabbccdd is the assertion that matters) or derive it from the field width; and reword the docstring so it does not advertise a layout the class will only have after #696.

5. EXPECTED_FAILURES removal ✅ genuinely passes

Imported: 43 entries (44 before), no key containing R1, remaining HIP entries only hip-parameter/HIP_TRANSFORM and hip-parameter/HOST_ID. test_round_trip_is_identity_or_a_recorded_gap fails an entry whose case has started passing, and the file is 6 passed / 360 subtests — so the case round-trips rather than being skipped, and no residual failure is masked.

6. Coverage ✅ claim holds, and the test is not coverage theatre

All three hip.py at 100% line and 100% branch from these two files, both with and without the new test:

selection	data/…/hip.py	internet/hip.py	schema/internet/hip.py
both files	118/0/0 100%	925/0/314 100%	384/0/28 100%
both files, new test deselected	100%	100%	100%

The new test adds zero lines and zero branches. That is the right outcome: the defect was the value of a class keyword argument on a line that executes at import either way, so line/branch coverage is structurally blind to it. The test discriminates behaviourally instead — §3 above proves it fails without the fix.

7. Minor — the test docstring's two-copy rationale does not reproduce

It says one copy "would raise ProtocolError: HIPv1: invalid format before dispatch is even reached and never exercise the registry lookup". Measured:

• this head: one copy does not raise. It parses, warns SchemaWarning: packet length < 0: -4, and yields counter=0 instead of 0xaabbccdd.
• main: one copy raises the AttributeError — i.e. it does reach the lookup.

The ProtocolError belongs to the round-trip harness's construct phase (#690's own table), not to the parse path this test builds by hand. Harmless to the fix, but it is presented as measured. Separately: that silent counter=0 on a 12-octet record is a quiet corruption path, and it is another reason to land #696.

Could not verify

• Full-suite coverage and the "100% before" figure on 8af2cfda7 itself — I was scoped to these two files and did not run the suite; the before/after table above is the same-selection substitute.
• fix(hip): correct R1_COUNTER's counter width and LOCATOR_SET's Length unit (#672) (#679) #696's own correctness beyond its two-line R1CounterParameter width change; I read its diff and merged it, but did not review it.
• Real HIPv1 captures. Every byte above is synthesised from the RFC diagrams, so conformance is checked against the spec, not against an implementation in the wild.

[JarryShaw]

Prior verdict superseded — rebased and corrected

The cross-review's ❌ NEEDS CHANGES (head 552ff9efa) is addressed. Summary of what changed:

• Rebased twice as origin/main moved during this fix: first onto 5f0a1aa90 (brings in
  fix(hip): correct R1_COUNTER's counter width and LOCATOR_SET's Length unit (#672) (#679) #696 — R1_COUNTER widened to 8 octets), then onto c496d9cb7 (HIP_COPIES can drop to 1 now that #672 and #679 have landed: measured 46 OK of 49 at both settings #689's later HIP_COPIES drop
  to 1). Both rebases were clean, no conflicts.
• Fixed the byte-literal at the old test_hip_unit.py:2356. The test now derives its expected
  wire bytes from R1CounterParameter.__fields__['reserved'].length and
  R1CounterParameter.__fields__['counter'].length instead of hardcoding
  0080000c00000000aabbccdd — so it no longer pins either width and won't break again if either
  moves.
• Reworded the docstring/PR-body claims the reviewer flagged as inaccurate: HIP.__parameter__
  is a hand-written dict (two literal entries), not a name-normalisation rule; the "two copies to
  dodge a ProtocolError" rationale didn't reproduce and is moot now that fix(hip): correct R1_COUNTER's counter width and LOCATOR_SET's Length unit (#672) (#679) #696 makes a lone record
  self-aligned — two copies are kept only to prove the reader's stride, matching the existing
  NAT/ESP full-parser test's own stated reason.
• Corrected the "does not collide with fix(hip): correct R1_COUNTER's counter width and LOCATOR_SET's Length unit (#672) (#679) #696" claim in the PR body: true of the diff (different
  lines within the same class), false of the merged behaviour (the old hardcoded literal broke
  against fix(hip): correct R1_COUNTER's counter width and LOCATOR_SET's Length unit (#672) (#679) #696's widened encoding). Both are now stated plainly in the PR body's "fix(hip): correct R1_COUNTER's counter width and LOCATOR_SET's Length unit (#672) (#679) #696 overlap"
  section.

New head: 32e996676137257b771863191cc4c6cba9c36533, on top of origin/main c496d9cb7
(verified mergeable: MERGEABLE, mergeStateStatus: BLOCKED — pending required checks, not a
conflict).

Tests on the rebased tree (PYTHONSAFEPATH=1, PYTHONPATH = this worktree,
pcapkit.__file__ confirmed rooted there): test_hip_unit.py 34 passed / 168 subtests,
test_option_roundtrip_unit.py 6 passed / 360 subtests — 40 / 528 total, unchanged from the
pre-review numbers. Falsified again post-rebase: reverting the schema fix alone reproduces
AssertionError: <class '...UnassignedParameter'> is not <class '...R1CounterParameter'> at the
registry-check assertion; restoring it (checksummed) passes.

Still one commit, same author/committer (Jarry Shaw <jarryshaw@icloud.com>), still unpublished
pending your review.

[JarryShaw]

❌ NEEDS CHANGES — cross-review on Opus of Sonnet-authored work: the fix, the derived test and the EXPECTED_FAILURES removal are all verified sound at head 32e996676 (the brief's f1b5b2a2f is a superseded pre-rebase head), and the merge-breaking defect from the first review is genuinely gone — but this PR falsifies prose in two files it does not touch, tests/protocols/internet/test_hip_r1_counter_width_unit.py:178-183 and examples/generators/options.py:977-989, the latter still carrying verbatim the "found by enumeration member name" claim this PR advertises as reworded throughout.

[JarryShaw]

Cross-review on Opus (authored by Sonnet). Head 32e996676 on c496d9cb7. Every probe restored via cp backup + sha256sum match, git status empty after each — no git checkout --, no git stash.
pcapkit.__file__ = /local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a7ff6edee37a1fbe3/pcapkit/__init__.py

#	Item	Verdict	Evidence I obtained myself
1	Derivation robust, not tautological	✅	counter → UInt32Field() makes the new test fail: b'\x00\x80\x00\x0c\x00\x00\x00\x00\xaa\xbb\xcc\xdd' != b'\x00\x80\x00\x08\x00\x00\x00\x00\xaa\xbb\xcc\xdd\x00\x00\x00\x00'. The derived content_len is cross-checked against _make_param_r1_counter's hardcoded len=12, so the two agree only at 4+8=12.
2	Still discriminates	✅	Reverting only code=[...]: AssertionError: <class '...UnassignedParameter'> is not <class '...R1CounterParameter'> at test_hip_unit.py:2354. Past that short-circuit the parse path on the test's own bytes gives AttributeError: 'UnassignedParameter' object has no attribute 'counter'.
3	Docstring 4 + 8	✅	reserved: PaddingField(length=4) + counter: UInt64Field(). Independently from RFC 5201: Type 128, Length 12, and §4.1.4 "monotonically increasing 64-bit counter" → 4+8. The inverse error was not introduced.
4	Three prose sites	✅	__parameter__ is defaultdict(lambda: 'unassigned', {...}) with its own literal line for 128 and for 129 (internet/hip.py:379-384); OptionField.unpack selects via schema = self._registry[code] (collections.py:442). Two-copy rationale measured: 1, 2 and 3 copies each parse cleanly, so it is a stride check, not a workaround.
5	Counts, and new test still present	✅	test_hip_unit.py 34 passed / 168 subtests (41.07s); test_option_roundtrip_unit.py 6 passed / 360 subtests. main has 33 def test_, head 34, name-diff is exactly one addition — nothing swapped out. Removal of the entry is required, not cosmetic: re-adding it yields 'OK' != 'PARSE' → SUBFAILED(case='hip-parameter/R1_Counter').
6	PR body	✅	States both the true textual fact and the false behavioural one; its counts match mine.

Item 1 answered directly: tautological on the 4/8 split alone — reserved=8/counter=4 produces byte-identical output at the chosen 0xaabbccdd. That residual is pinned by tests/protocols/internet/test_hip_r1_counter_width_unit.py (on main), which I ran under the same mutation: 9 failed / 3 passed, AssertionError: 12 != 16, plus ProtocolError: HIPv2: invalid format; its test_the_counter_carries_a_64_bit_value exercises 0xffffffffffffffff, which a 32-bit field cannot hold. So the author's "that is #696's test's job" argument stands.

What needs changing — prose only, no code:

• tests/protocols/internet/test_hip_r1_counter_width_unit.py:178-183 still explains that code 128 "fails … R1CounterParameter declares only 129, so 128 parses as an UnassignedParameter", and cites hip-parameter/R1_Counter in test_option_roundtrip_unit as the record. This PR makes all of that false and deletes that entry.
• examples/generators/options.py:977-989 and 1004: "Three remain" is now two (measured — hip-* entries are HIP_TRANSFORM, HOST_ID), the R1_Counter bullet cites the deleted entry, and line 986 still reads "_read_param_* and _make_param_* are found by enumeration member name" — the exact claim removed from the three other sites.

Could not verify: CI (every check QUEUED bar pyup.io/safety-ci); the body's 100%-coverage claim (no coverage run); RFC 5201 §5.2.3's diagram verbatim (page truncated — used its parameters table and §4.1.4); the full suite, not run as briefed.

Minor, no action: all 49 __parameter__ values do equal code.name.lower(), and a getattr(self, f'_read_param_{name}') step does exist downstream (internet/hip.py:750-752) — so the old wording was mechanically wrong but behaviourally indistinguishable, making the correction right but less consequential than the body implies.

[JarryShaw]

Second cross-review addressed — prose-only, no code change

Both flagged sites fixed, rebased onto origin/main 4391dc77b (#720, unrelated to these files).

test_hip_r1_counter_width_unit.py:178-183 now reads:

Code 128 is deliberately not exercised here. #690 registered R1CounterParameter for 128 as well
as 129, so it now reaches this same class rather than parsing as an UnassignedParameter — that
coverage lives with the fix, in HIPUnitTests.test_hip_r1_counter_code_128_resolves_to_its_own_schema,
not here; this module stays scoped to the width #672 fixed.

options.py: "Three remain" → "Two remain" (HOST_ID, HIP_TRANSFORM); R1_Counter's bullet
replaced with a paragraph explaining it closed separately via #690, and the "found by enumeration
member name" claim reworded to "hand-written dict entries... not a name-normalisation rule."
Verified directly: 47 OK of 49 HIP codes at both one and two copies (was 46/46 pre-#690).

The len=12 mechanism — added as a comment plus explicit assertEqual(content_len, 12), since
it's load-bearing: confirmed the mutation counter: UInt32Field() now fails there
(AssertionError: 8 != 12) rather than silently agreeing.

New head: 818e179695b37bde39a68062e6f1ab1b68a8c8bb. Tests: test_hip_unit.py 34/168,
test_option_roundtrip_unit.py 6/360 (40/528 total) — unchanged counts.

[JarryShaw]

✅ GOOD TO MERGE @ 818e17969 — HIPv2 cannot regress: RFC 7401 §9 leaves code 128 assigned to "R1_Counter (v1 only)" and never reuses it, and HIPv2+128 raises the identical ProtocolError before and after the fix; one judgement call for you: HIP's own published parameter table still omits R1_Counter.

[JarryShaw]

✅ GOOD TO MERGE @ 818e17969 — HIPv2 cannot regress: RFC 7401 §9 leaves code 128 assigned to "R1_Counter (v1 only)" and never reuses it, and HIPv2+128 raises the identical ProtocolError before and after the fix; one judgement call for you: HIP's own published parameter table still omits R1_Counter.

Cross-review #3, on Opus (authored by Sonnet). The 22:22Z ❌ verdict referred to 32e996676 and is superseded — both sites it named are now accurate, not merely edited; verified below. Read-only: every probe ran in throwaway copies under /tmp/xr721/, this worktree was never mutated and no git merge was run.

#	Claim	Verdict	Evidence I obtained
1	HIPv2 unharmed	✅	RFC 7401 §9 verbatim: "the existing value (128) for R1_COUNTER has been left in place with a reference to [RFC5201] … the name for the value 128 has been changed from 'R1_COUNTER' to 'R1_Counter (v1 only)'"; §5.2.3 assigns Type 129, §10 gives the C-bit reason. Measured on both trees: v2+128+len12 → ProtocolError: HIPv2: [ParamNo 128] invalid parameter identically before and after (guard, pcapkit/protocols/internet/hip.py:879); v2+129 unchanged.
2	No stale hardcoded width	✅	Mutated counter → UInt32Field(): the new test fails loudly, AssertionError: 8 != 12 at test_hip_unit.py:2373 — the surviving 12 is an assertion against _make_param_r1_counter's own len=12 (internet/hip.py:3034), not a stale literal. Residual reproduced: reserved=8/counter=4 leaves the new test green; that split is pinned by test_hip_r1_counter_width_unit.py, 4 failed under the same mutation (b'\x00\x00\x00\x00\xff\xff\xff\xff' != b'\xff…').
3	EXPECTED_FAILURES hides nothing	✅	Imported: 43 entries, HIP keys only HIP_TRANSFORM/HOST_ID, no R1. Byte-identical, not merely non-fatal: status=OK warnings=() octets=…0080000c0000000000000000aabbccdd (MISMATCH is a separate status). Re-adding the entry → SUBFAILED(case='hip-parameter/R1_Counter') "came back OK", so the removal was required.
4	Both flagged prose sites now accurate	✅	options.py: I measured all 49 HIP codes — 47 OK at one copy, 47 OK at two, failures exactly HIP_TRANSFORM ([ParamNo 577] invalid parameter) and HOST_ID; make_samples.py independently reports the same two and no R1_Counter. width_unit.py:178-183: the method it names exists and passes, and tests/ is absent from the Sphinx tree, so the bare :meth: target matches that module's own existing style.
5	No undeclared behaviour change	⚠️ two, both benign	Malformed short 128 records now emit an extra SchemaWarning: packet length < 0: -8 before the same ProtocolError … invalid format (measured both trees) — fixed-width padding vs UnassignedParameter's len-sized value; 128 now behaves as 129 always did. Third-party EnumSchema registration of 128 now warns schema 128 already registered where it was silent. Neither is breaking; fix is right.

Merge safety, established without merging. main is one commit ahead of the base (daa953d1b, PCAP-NG only), disjoint from all five files; overlaying its two files onto this head → 44 passed / 538 subtests, tree /tmp/xr721/vsmain/pcapkit/__init__.py. The sibling worth checking is #726 — the only other open PR touching schema/schema.py, i.e. the for _code in codes: loop code=[...] goes through, same base 4391dc77b so no textual conflict, exactly the shape of the trap the first review caught. Its diff only loosens that guard to incumbent is not cls and leaves the loop intact; #721's five files overlaid on #726's tree → 56 passed, tree /tmp/xr721/merged/pcapkit/__init__.py.

As shipped, pcapkit.__file__ = …/worktrees/agent-acea9b17dbd2fe46a/pcapkit/__init__.py, CPython 3.14.7, PYTHONSAFEPATH=1: test_hip_unit.py 34 passed / 168 subtests; test_option_roundtrip_unit.py + test_hip_r1_counter_width_unit.py 10 passed / 370 subtests (6/360 + 4/10). CI rollup SUCCESS, 26 contexts, 24 pass / 2 skipping. mergeStateStatus: BEHIND — needs an update-to-main if up-to-date branches are required.

The judgement calls, neither a blocker.

• pcapkit/protocols/internet/hip.py:259 — HIP's published docstring claims it "currently supports parsing of the following HIP parameters" and lists 48 of the 49 __parameter__ codes; the sole omission is R1_Counter. It is one row per distinct parser (48 rows, 48 distinct _read_param_*, r1_counter the only shared value), so it is self-consistent and the omission predates this PR — but this PR is what makes 128 parse, so that under-claim is now the last inaccurate thing in the rendered docs. Your call whether it earns the duplicate-parser row.
• schema/internet/hip.py:570 credits the 8-octet widening to fix(hip): correct R1_COUNTER's counter width and LOCATOR_SET's Length unit (#672) (#679) #696, while the counter docstring ten lines below says "It was a UInt32Field until R1_COUNTER packs 12 octets where RFC 7401 §5.2.3 requires 16: counter is 4 octets, not the stated 8 #672" and width_unit.py says "the width R1_COUNTER packs 12 octets where RFC 7401 §5.2.3 requires 16: counter is 4 octets, not the stated 8 #672 fixed". Landed as c70c45c5e … (#672) (#679) (#696) — issues then PR, so both are defensible; they just now disagree inside one class docstring.

Could not verify: the full suite (only the four named files, as briefed); the coverage claim (no coverage run); real HIPv1 captures — every octet above is synthesised from the RFC diagrams, so this is conformance to the spec, not to a deployed implementation.

✅ GOOD TO MERGE @ 818e17969 — HIPv2 cannot regress: RFC 7401 §9 leaves code 128 assigned to "R1_Counter (v1 only)" and never reuses it, and HIPv2+128 raises the identical ProtocolError before and after the fix; one judgement call for you: HIP's own published parameter table still omits R1_Counter.