[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort): isort and mypy clean on changed files; full pylint not run
• make test passes, and a test case covers the change: targeted unit tests only, full suite not run
• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible: N/A, centralised in chore(docs): centralize 1.6.0 changelog updates in one place, not per-feature-PR #2449

What is the purpose of your pull request?

• feat: adds a feature

---

Description of your pull request and other information

Implements ICMP (RFC 792) and ICMPv6 (RFC 4443) under an abstract ICMP base, whose id() is ('ICMP', 'ICMPv6').
ICMPv4 and ICMPv6 share the Echo, Destination Unreachable and Time Exceeded bodies in the base; ICMPv6 adds Packet Too Big. Unknown types stay raw.
Checksums are kept as-is, never verified or computed (#2470); Neighbor Discovery and MLD stay raw. register_icmp_type is the registry wrapper.
Tests: byte-exact round trips, selector tests on parsed frames, and registry routing.
Closes #2436

[JarryShaw]

NEEDS CHANGES (cross-review, different model from the author).

Required: (1) the 8 const/vendor pairs are hand-typed literals that have drifted from live IANA — several real assigned values (e.g. ICMPv6 Extended Echo 160/161, RFC 8335; several Destination Unreachable/Parameter Problem codes) come back Unassigned, and ICMP type 10's label is wrong (Router_Selection vs IANA's "Router Solicitation"); switch all 8 to real LINK CSV crawlers, as 88 existing crawlers already do, so the vendor cron can actually track IANA. (2) ICMP()/ICMPv6() can't be constructed with no arguments — raises ProtocolError, unlike 32 other protocol classes; default body should be {}/None. (3) pep.rst has several factual errors: "first instance of the shared-base pattern" contradicts its own "following IP" in the same sentence; an unrelated paragraph still calls MLD a wave-2 deliverable, contradicting the new "follow-up" framing; cites RFC 3810 which RFC 9777 obsoleted; no follow-up issue actually exists for ND/MLD/checksum despite the prose implying one. (4) Checksum is unimplemented — issue #2436 explicitly asked each class to implement its own checksum independently (ICMPv6 needs the IPv6 pseudo-header); the PR computes/verifies neither, so "Closes #2436" overclaims. (5) Missing 1.6.0 changelog bullet (main opened the file after this branch's base — needs a rebase).

Verified correct: real-packet (scapy, valid checksums) round-trips for both protocols across 17 message types including every error type, all byte-exact; dispatch wiring (proto 1→ICMP, 58→ICMPv6, HOPOPT passthrough); shared ICMP_Family base matches the IP/IPsec precedent; 512 passed/24,757 subtests; scope clean across all 67 files; no citation-convention or counted-number violations.

Non-blocking: doc says "three fields" lists four; a false claim that Raw is the only protocol with no payload field; code fields are bare int not wired to their IntEnum; and a cross-PR clash — this PR's test for an unregistered transport-type number reuses IGMP(2), which #2444 is about to register, so whichever lands second needs to pick a different number (253 recommended, RFC 3692).

[github-actions]

Coverage: 94.06% (unit tier, Python 3.15, a247e314e, Unit Tests run success)

Package	Statements	Missed	Branches	Partial	Cover
pcapkit (top level)	104	4	20	2	95.16%
pcapkit/const	20008	142	2686	123	98.79%
pcapkit/corekit	2605	74	818	37	96.52%
pcapkit/dumpkit	281	3	100	3	98.43%
pcapkit/foundation	3194	27	1112	31	98.65%
pcapkit/interface	138	4	52	3	96.32%
pcapkit/protocols	18875	199	4922	193	98.34%
pcapkit/toolkit	924	36	334	10	95.55%
pcapkit/utilities	502	7	144	5	98.14%
pcapkit/vendor	4826	2113	1108	177	52.95%

Per-file detail: the coverage-html artifact of this run.

[JarryShaw]

All five required fixes applied and verified directly (new head b627627ea): all 8 vendor crawlers converted from hand-typed literals to real IANA CSV crawlers (confirmed: ICMP type 10 now "Router Solicitation", type 8 corrected to "Echo" per the real registry); ICMP()/ICMPv6() now construct with zero arguments; pep.rst's factual errors corrected (RFC 9777 for MLD, ND/NDP reconciled); a new pcapkit.utilities.checksum module added and wired into both protocols' make/read (ICMPv6's pseudo-header threaded through __packet__ context, matching this codebase's existing convention); changelog bullet added. Spot-verified: both classes construct cleanly, 616 passed/26896 subtests on the targeted suites.

One operational note: I discovered mid-review that the ORIGINAL worker agent was still running in this same worktree 3+ hours after opening the PR, concurrently with the fix-relay I'd dispatched — a real collision the relay detected and reconciled (kept two beneficial concurrent edits, discarded one cosmetic one). I've stopped the stray worker; the pushed state is independently verified clean. Cross-review dispatched.

[JarryShaw]

NEEDS CHANGES (cross-review round 2, different model from the author).

Checksum arithmetic itself is correct (8 ICMP message kinds and 7+ ICMPv6 paths independently verified against scapy and an RFC 1071 reference loop). But the verification logic produces false "invalid checksum" warnings on VALID traffic in three real situations, and silently drops ICMPv6-over-IPv4 to Raw entirely:

Required: (1) false positives on valid packets — the first fragment of a fragmented ICMP/ICMPv6 message warns (checksum covers the whole reassembled message, only the first fragment gets summed; large pings and PMTU probes hit this constantly), a truncated/short-snaplen capture warns on every echo, and an IPv6 Routing header with Segments Left > 0 warns because the pseudo-header should use the FINAL destination per RFC 8200 §8.1 (which the PR itself cites) but uses the IPv6 header's immediate dst instead — Wireshark explicitly skips verification in the first two cases. (2) ICMPv6 reached over IPv4 (protocol 58) crashes to Raw — IPv4 also populates __packet__'s src/dst, so the guard passes, then ipaddress.IPv6Address(an IPv4Address) raises AddressValueError and the whole layer is lost; fix by checking both addresses are actually IPv6 before attempting pseudo-header verification. (3) three docstrings still say "neither this class nor the other verifies or computes [the checksum]" — now false — plus one stale RFC 3810 citation in icmpv6.py that pep.rst already corrected to RFC 9777 elsewhere.

Minor: inet_checksum's fold loop runs exactly twice instead of looping until done (RFC 1071), giving a wrong result only on ICMPv6 jumbograms past 131077 octets — found because 4 other RFC 1071 implementations already exist in examples//tests/ (so "no prior art" was only half true for the library itself, not the repo) and all of them loop correctly, which is how the bug surfaced; fix: while total >> 16:. Test fixtures in examples/generators/dispatch.py build ICMP/ICMPv6 with a guaranteed-invalid zero checksum (noise, not a defect).

Everything else — IANA registry accuracy, __packet__ threading (confirmed existing mechanism, not new), ProtocolWarning shape matching precedent, docs toctree completeness, pep.rst claims — verified clean. Relaying the required fixes.

[JarryShaw]

All required fixes applied and verified directly (new head 27f179477): checksum verification now skips (never warns) on a first fragment, a truncated capture, or (ICMPv6) an IPv6 Routing header with Segments Left > 0 — threaded via a new mf/route_seg_left key in __packet__; ICMPv6-over-IPv4 now explicitly checks isinstance(src/dst, IPv6Address) before building the pseudo-header instead of just key presence, so it degrades gracefully instead of crashing to Raw; three stale docstrings and the RFC 3810→9777 citation fixed; inet_checksum's fold now loops correctly (confirmed wrong at exactly the jumbogram boundary scapy flagged). Spot-verified: 45 passed/14 subtests on the new regression tests. Cross-review re-dispatched.

[JarryShaw]

NEEDS CHANGES (cross-review round 3, different model from the author) — round 2's three bugs (fragmentation false-positive, ICMPv6-over-IPv4 crash, fold-loop arithmetic) are all genuinely fixed, confirmed by exhaustive re-derivation (26 inputs across the fold boundary, 3-fragment dispatch trace confirming middle/last fragments never even reach ICMP's read(), zero key collisions with the 4 existing __packet__ readers).

Two new edge cases found, both in the same checksum-skip logic:

Required: (1) Mobile IPv6 Home Address option (RFC 6275 §11.3.1) gives a false "invalid checksum" warning — a real ICMPv6 sender using Mobile IPv6 computes the checksum using the home address as the pseudo-header source, but the wire source is the care-of address; since pcapkit already parses this option (IPv6_Opts._read_opt_home), the IPv6 walk should flag it (same skip-rather-than-recompute pattern already used for the Routing Header case) or use the home address directly. (2) a malformed Routing header bypasses the route_seg_left flag entirely — when the Routing-header parser rejects a header and falls back to the generic IPv6_Ext parser, that generic info has no seg_left key, so info.get('seg_left', 0) silently reads 0 and the flag never gets set, producing the same false warning round 2 was supposed to close for exactly this case.

Non-blocking: route_seg_left (and by extension mf) is never cleared between nested IPv6 layers, so an SRv6-encapsulated inner datagram's genuinely corrupted checksum could be silently skipped rather than flagged (missed detection, not a false positive — lower severity, but worth a follow-up); strict=False reassembly warns on intentionally-partial datagrams (expected but worth documenting); one untested branch in the fold padding logic; the ICMP-over-IPv4 happy-path test ignores warnings so nothing actually proves the silent case.

Relaying the two required fixes.

[JarryShaw]

Both required fixes applied and verified directly (new head de614f6b8): a Destination Options Home Address option now threads the real home address through __packet__, and ICMPv6.read uses it as the pseudo-header source when present (RFC 6275 §11.3.1); a Routing header that fails structured parsing now reads Segments Left directly from the raw bytes (ext_payload[3]) instead of defaulting to 0 when the key is absent, defaulting to the conservative skip if even that's too short. Spot-verified: both new regression tests pass. Cross-review re-dispatched.

[JarryShaw]

NEEDS CHANGES (Sonnet, ba0d43220). All eight fixes hold. Docs warnings equal main's, selectors behave as ruled, and 20,480 random round trips show 0 mismatches.

• test_internet_unit.py now says IGMP has an Internet.__proto__ entry. It doesn't on this branch, so name only ICMP.
• ICMP/ICMPv4/ICMPv6 are missing from the "protocol classes pcapkit ships" table in ext.rst and from the protocols/index.rst mermaid hierarchy and its click list.
• Nit: pep.rst says checksums are never "verified"; they're never computed either.

[JarryShaw]

Verified (35aa25137): docs/prose round as listed above. Sonnet review dispatched.

[JarryShaw]

GOOD TO GO (Sonnet, 35aa25137). Docs-only round (empty pcapkit diff); the ICMP Family table and mermaid subgraph mirror IP; docs warnings equal main's; ICMP tests pass.

[JarryShaw]

Reads like there's an overlap in the ICMPv4 and ICMPv6's implementation? Can we move some of those to the base ICMP class?

[JarryShaw]

I think there're similar/duplicate that can be extracted from ICMPv4/6 classes into ICMP base class. We can use dynamic attributes for the error messages and names. Also, we could use an abstract property @version in ICMP base class then return 4/6 in ICMPv4/6 respectively.

[JarryShaw]

My reading of your 03:42Z comment: move the duplicated ICMPv4/ICMPv6 code into the ICMP base; use dynamic attributes for the error messages and names; and an abstract @version on ICMP, returning 4 and 6 in ICMPv4 and ICMPv6. This is a rework, so the PR goes back to needs-changes; the fix round is dispatched.

[JarryShaw]

Also, seems like we need to add registry method for the __type__ registry in ICMPv6.

[JarryShaw]

cross reference #2444 for the convention rulings.

[JarryShaw]

My reading: add a register_type helper to pcapkit.foundation.registry.protocols, and have ICMP.register_type delegate to it, so the type-registry logic lives in the shared registry. This is a rework, so #2446 goes back to needs-changes; the fix round is held until the Actions queue is below 40.

[JarryShaw]

Held: Actions queue at 47 (above the hold line). The shared register_type rework dispatches when the queue drops below 40.

[JarryShaw]

Also for the held rework (review of fdd82c704): docs/source/contributing/pep.rst "Several of these come in pairs" still lists ICMP with ICMPv6 as wanting a shared abstract base, and says IP and ICMP already cover their families. Drop the ICMP with ICMPv6 pair, as the earlier conflict resolution should have.

[JarryShaw]

Correction to my reading: add a wrapper method in pcapkit.foundation.registry, like the other protocol wrappers there, and have ICMP.register_type call it. The rework stays held until the Actions queue is below 40.

[JarryShaw]

NEEDS CHANGES (Sonnet, e2cab4ffb). 85,873 records are identical to fdd82c704; ICMP() is abstract; the version and dispatch values are right.

• icmp.py 19-21: NOTE is 3 lines; drop the file-level mypy suppressions (17-18) by declaring __type__ and the none hooks on the base.
• Body: 18 non-blank lines; trim to 15.
• icmpv4.py/icmpv6.py: 8 identical methods (_read/_make_data_type_{none,echo,destination_unreachable,time_exceeded}) remain; move them into the base or justify.
• Test test_make_error_messages_use_the_shared_alias leaks two SchemaWarnings; wrap in warnings.catch_warnings().

[JarryShaw]

Correction, you were right: keep ICMP.register_type as the classmethod that stores the entry, and add a foundation-registry wrapper in pcapkit.foundation.registry.protocols (like register_ipv4_option) that validates the code and calls ICMP.register_type. The rework stays held until the Actions queue is below 40.

[JarryShaw]

Member order is in scope for this rework. Group the class body as: properties, public methods (id, read, make, register_xxx), dunders (__index__, __length_hint__), then utilities (_read_xxx before its _make_xxx). Use the old protocols (IPv4, TCP, UDP, ARP) as the reference.

[JarryShaw]

My reading of your comment: icmp.py has extra blank lines; drop the _XXX class attributes that carry the dynamic names and aliases; and revert the shared-base extraction so ICMPv4 and ICMPv6 stay separate, as in the earlier design. This is a rework, so #2446 goes back to needs-changes. The local fix is held until the Actions queue is below 40.

[JarryShaw]

Local-only commit 79a5003 is a superseded ICMP shape; the rebase onto origin fa93a48 conflicts across about 50 files. Blockers: (1) whether origin verifies ICMP checksums (now togglable per the owner); (2) the pseudo-header work in #2479. Not pushed.

[JarryShaw]

Ruling applied: ICMP checksum verification and calculation are togglable, not always on, and wait for the pseudo-header work. Local commit 79a5003 is not the branch for that work, so it stays unpushed. Blocked until #2479 merges, then #2470 starts from there. Nothing to decide here now.

[JarryShaw]

Rebased onto main after #2443/#2491 landed, new head 1c2c01c4c. Conflicts in pep.rst, pcapkit/all.py and 5 test-count files, all resolved by merging both sides. Narrow tests: ICMPv4 18 passed, ICMPv6 20 passed, selector 4 passed (42 total). The existing needs-changes items are unrelated to this rebase and still apply; label unchanged.

[JarryShaw]

Rebased onto main after #2497 (ARPHRD crawler) landed, new head 9c56f7101. Conflict was the vendor const-census count in test_vendor_missing_body_unit.py; verified the real count (42, not a guessed bump) and set it. Narrow tests: 48 passed, 118 subtests. Existing needs-changes items are unrelated to this rebase and still apply.

[JarryShaw]

Applied the rework ruled on at 2026-10-31T05:41:43Z (new head bc90e23a1):

• Dropped the shared-base extraction. icmpv4.py/icmpv6.py are independent
  again, as in the design before the 03:42Z ask: each owns its own name/alias/
  length properties, read/make, register_type, and all its
  _read_data_type_*/_make_data_type_* message bodies. icmp.py is back to a
  minimal abstract base holding only id() (('ICMP', 'ICMPv6')).
• Dropped the _XXX class attributes (_NAME, _ALIAS, _INDEX,
  _TYPE_SCHEMA, _DATA_CLASSES, _SCHEMA_CLASSES) and the abstract version
  property that carried the dynamic names/aliases for the extraction.
• Fixed icmp.py's extra blank lines — gone with the revert to the plain
  pre-extraction body.
• Applied the 2026-10-31T04:29:19Z member-order ruling: properties →
  methods (id, read, make, register_type) → dunders (__length_hint__,
  __index__, matching IPv4/TCP/UDP/ARP's actual order) → utilities (all
  _read_data_type_* before all _make_data_type_*, ascending by type code,
  fallback first — matches HOPOPT's option-ordering precedent).
• pep.rst (2026-10-31T04:06:39Z): dropped "ICMP with ICMPv6" from the
  "Several of these come in pairs" wanting-list, and corrected "Done within
  wave 2 — ICMP/ICMPv6 shared base" to stop claiming a shared message-type
  method body (it's selector composition only now).
• Preserved: id() composition (ICMP→('ICMP','ICMPv6'),
  ICMPv4→('ICMP',), ICMPv6→('ICMPv6',)), the register_icmp_type
  foundation-registry wrapper (fixed its stale ICMP.register_type docstring
  reference to point at each class's own register_type), and the
  checksum-deferral state (untouched; checksum logic was already fully
  stripped before this rework).
• Removed two tests for the now-reverted version property
  (test_version_is_{4,6}_and_abstract_on_icmp).

Tests: narrow ICMP suite 44 passed / 32 subtests (ICMPv4=17, ICMPv6=19,
selector=4, registrar=4 — net even with the 42-test baseline after removing
the 2 obsolete version tests). tests/project/ (member-order, docs-claims,
isort, …): 2132 passed, 80 skipped, 10623 subtests, 0 failed. isort clean
on the touched pcapkit/ files.

[JarryShaw]

Cross-review (Sonnet) at bc90e23a1: the revert is complete and correct (byte-identical icmp.py to the pre-extraction 35aa25137, member order/id() values/checksum-deferral state all confirmed unchanged). Only issue: census pins short by 1 (ICMP+ICMPv6 add 8 const modules, bumps were +7). Pushed ea602970a: bumped the remaining +1 across all 4 census files. Verified: tests/const/, tests/vendor/ pass clean (580 tests, 27053 subtests). Re-review owed on this head.

[JarryShaw]

Cross-review (Sonnet) at ea602970a: GOOD TO GO. Diff since the last round touches only the 4 census test files, pins consistently +8 against main matching the 8 new const modules (4 under icmp/, 4 under icmpv6/, plus 8 matching vendor modules -- generated moves 1:1 with all_files since nothing else changed). Full suite clean: tests/const/ 390/24792 subtests, tests/vendor/ 190/2261 subtests, narrow ICMP suite 44/32 subtests, tests/project/ 2132/10623 subtests, all 0 failures. Ready for merge -- your call.

[JarryShaw]

Rebase onto origin/main

Old head: ea602970a → New head: a247e314e

Rebased onto origin/main to pick up IGMPv1/v2 (#2444), DTLS reassembly (#2451) and the
ARPHRD vendor docs (#2505). All 19 conflicting files were "both sides added entries to
the same shared list/registry" (export lists, doc tocrees, registry dicts) or doc tocrees;
resolved by keeping both sides' additions, alphabetically where the file already sorted
that way. docs/source/contributing/pep.rst's three prose conflicts (outstanding-protocol
list, and two wave-2 summary paragraphs) were merged for meaning rather than concatenated:
both ICMP/ICMPv6 and IGMP are now described as done, leaving ECN/Shim6 and MLD/IGMPv3 as
the remaining gaps.

Pins — measured, not computed

The 5 census-pin files were conflicts between two different absolute counts (this PR's
ICMP/ICMPv6 additions vs. main's already-merged IGMP additions). Rather than add the two
sides' deltas by hand, I resolved the conflict, ran each test, and read the actual
AssertionError: <measured> != <pinned> to get the true count — then a second pass of the
same measure-fix-rerun loop was needed because the rebase's own second commit
(fix(tests): bump the census pins for ICMP/ICMPv6's 8 new enums) applied cleanly on top
of my merge but landed every count exactly 1 low. Final measured values:

file	assertion	value
test_const_enum_builtin_parity.py	len(ints)	126
	len(flags)	6
	len(strs)	10
	len(self.enums)	142
	len({obj.__module__ ...})	136
test_const_enum_get.py	len(self.enums)	132
	covered	129
test_const_enum_lookup.py	len(self.enums)	126
test_const_registry_protocol.py	len(all_files)	136
	len(generated)	119
test_dispatch_registry_unit.py	len(cases)	56
	by_family['internet']	20

Tests (on the final amended head, PYTHONSAFEPATH unset — see below)

• tests/const/: 390 passed, 24850 subtests passed
• tests/vendor/: 190 passed, 2268 subtests passed
• Narrow ICMP/IGMP/dispatch (test_icmpv4_roundtrip_unit, test_icmpv6_roundtrip_unit,
  test_icmp_selector_runtime, test_icmp_type_registrar_unit, test_igmp_unit,
  test_dispatch_registry_unit): 91 passed, 230 subtests passed
• tests/project/: 2132 passed, 80 skipped, 0 failed, 10687 subtests passed

One red herring worth recording: an initial tests/project/ run showed 234 unrelated
failures (CI workflow-script tests — benchmark matrix, apt cache, python-versions,
superseded-run reporting). Root cause was not this rebase or host contention — it was
my own PYTHONSAFEPATH=1, used to confirm which tree pcapkit resolved into, leaking
through os.environ into the subprocess calls those tests make to util/*.py scripts,
which broke those scripts' own sys.path setup. Confirmed by re-running one failing test
with PYTHONSAFEPATH unset (passed), then the full suite (clean, 0 failed).

git merge-tree --write-tree HEAD origin/main confirms a clean tree with no conflicts.

One commit per package, two commits total (the original feat(internet) + fix(tests)
pair), force-pushed with --force-with-lease. No content changed beyond the conflict
resolution and the pin values above — this was a rebase, not a content review, so the
review: good-to-go label is left as-is pending a confirming pass given the pin-test
content did change.

[JarryShaw]

Cross-review (Sonnet) at a247e314e: GOOD TO GO. Diff against main confirmed scoped to ICMP/ICMPv4/ICMPv6 plus shared list/registry entries -- no IGMP content touched. No conflict markers. Census pins reproduced by running the real tests (not trusted from the rebase report): tests/const/ 390/24850 subtests, tests/vendor/ 190/2268, IGMP+dispatch 47/198, narrow ICMP 44/32, tests/project/ 2132/10687, all 0 failures. IGMP (merged in via the rebase) and ICMP both verified parsing/dispatching correctly end to end with real packets. mergeStateStatus: BLOCKED is just the pending-review gate, not a conflict. Ready for merge -- your call.