Skip to content

fix(internet): narrow IGMP's id() to self, per owner's post-merge ruling - #2506

Merged
JarryShaw merged 2 commits into
mainfrom
fix/2444-igmp-id-narrow-to-self
Nov 1, 2026
Merged

fix(internet): narrow IGMP's id() to self, per owner's post-merge ruling#2506
JarryShaw merged 2 commits into
mainfrom
fix/2444-igmp-id-narrow-to-self

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 31, 2026

Copy link
Copy Markdown
Owner

Post-merge correction to #2444, per owner ruling: IGMPv1/IGMPv2 shipped with the family-inclusive id() shape (('IGMP', 'IGMPv1')), but the ruling is that IGMP narrows to self, like IP/IPsec's AH and ESP.

  • IGMP.id() -> ('IGMPv1', 'IGMPv2') (members only, drops the family name)
  • IGMPv1.id() -> ('IGMPv1',), IGMPv2.id() -> ('IGMPv2',)

Selector matching for the bare 'IGMP' string is unaffected -- it resolves via the registry key's own expansion, the same mechanism that already makes 'IPsec' select an AH or ESP layer. Verified directly against a real parsed IGMPv1 layer.

Recorded the ruling in protocol-family-grouping.rst, which previously had no IGMP entry.

Build and test: tests/protocols/internet/test_igmp_unit.py, test_dispatch_registry_unit.py, test_conventions_doc_claims.py, test_public_api.py all pass clean (91 tests, 1163 subtests).

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 31, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 31, 2026
@JarryShaw
fix(internet): IGMPv1 narrows id()/alias to the RFC canonical name
711141b 
Follow-up to the owner's ruling on this fix: IGMPv1 (RFC 1112) should
take the same shape as ICMPv4 (PR #2446) -- its RFC's own name carries
no version, so id() and alias both drop it, rather than narrowing to
a versioned 'IGMPv1' name that RFC 1112 never uses.

- IGMPv1.id() -> ('IGMP',), IGMPv1.alias -> 'IGMP'.
- IGMPv2.id() -> ('IGMPv2',) and IGMPv2.alias -> 'IGMPv2', unchanged:
  RFC 2236 names IGMPv2 with its own version, so it keeps the shape
  from the first round of this fix.
- IGMP.id() (the family base) -> ('IGMP', 'IGMPv2'), the
  family-inclusive shape, since IGMPv1's canonical name and the
  family's own name are now the same string -- the same shape
  ICMP.id() takes, ('ICMP', 'ICMPv6').
- igmp.py's shared read() error message kept the version-specific
  'is not an IGMPv1/IGMPv2 message' wording explicit, decoupled from
  alias, so the error stays as informative as before despite the
  alias change.

Selector matching for the bare 'IGMP' and 'IGMPv1' strings is
unaffected -- 'IGMPv1' still has its own registry key independent of
id(), the same mechanism ICMPv4 relies on for 'ICMPv4'.

Promoted the ICMPv4 doc text from 'a proposal, not a ruling' to a
settled ruling (PR #2446 shipped exactly this shape), and recorded
IGMPv1's matching case alongside it.

Verified: tests/protocols/internet/test_igmp_unit.py (39 tests),
test_dispatch_registry_unit.py, test_conventions_doc_claims.py and
test_public_api.py all pass clean (91 tests, 1163 subtests).
@JarryShaw
JarryShaw force-pushed the fix/2444-igmp-id-narrow-to-self branch from 5d918b1 to 711141b Compare October 31, 2026 21:17
@JarryShaw

JarryShaw commented Oct 31, 2026

Copy link
Copy Markdown
Owner Author

Pushed 711141b3a: follow-up to the ruling per your direct correction. IGMPv1 (RFC 1112) takes ICMPv4's shape, not plain self-narrowing: id() and alias both drop the version since the RFC's own name carries none.

  • IGMPv1.id() -> ('IGMP',), IGMPv1.alias -> 'IGMP'.
  • IGMPv2.id() -> ('IGMPv2',), unchanged (RFC 2236 names it with a version).
  • IGMP.id() (base) -> ('IGMP', 'IGMPv2'), family-inclusive, same shape as ICMP.id() = ('ICMP', 'ICMPv6').
  • Decoupled read()'s error message from alias so it keeps saying 'is not an IGMPv1/IGMPv2 message' explicitly rather than losing version-specificity.

Also promoted protocol-family-grouping.rst's ICMPv4 text from 'a proposal, not a ruling' to settled, since #2446 shipped exactly this shape, and recorded IGMPv1's matching case alongside it.

Verified: test_igmp_unit.py (39 tests), test_dispatch_registry_unit.py, test_conventions_doc_claims.py, test_public_api.py all clean (91 tests, 1163 subtests).

@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet labels Oct 31, 2026
@JarryShaw

JarryShaw commented Oct 31, 2026

Copy link
Copy Markdown
Owner Author

Cross-review (Sonnet) at 711141b3a: GOOD TO GO. __eq__/registry mechanics confirmed exactly as claimed (string comparison only ever checks id(), never cls.__name__; 'IGMPv1'/'IGMPv2' have independent registry keys). One surfaced behavior worth noting, not a defect: 'IGMP' in an IGMPv2 layer is now True -- the family-inclusive base id() (('IGMP', 'IGMPv2')) makes the bare family selector match both versions via registry-key expansion, same as 'ICMP' in an ICMPv6 layer would under #2446's identical shape. Error messages, docs text, and the ICMP-precedent analogy all checked out directly. test_igmp_unit.py and test_dispatch_registry_unit.py clean (47 tests, 194 subtests); tests/project clean apart from 2 pre-existing, unrelated sandbox-network failures confirmed present on main too. Ready for merge -- your call.

@github-actions

github-actions Bot commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Coverage: 94.47% (unit tier, Python 3.15, 711141b3a, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19738 133 2642 120 98.85%
pcapkit/corekit 2605 74 818 37 96.52%
pcapkit/dumpkit 281 3 100 3 98.43%
pcapkit/foundation 3183 27 1110 31 98.65%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 18471 199 4860 184 98.35%
pcapkit/toolkit 924 36 334 10 95.55%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4510 1888 1034 169 54.89%

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

@JarryShaw JarryShaw mentioned this pull request Oct 31, 2026
Merged
5 tasks done
@JarryShaw
Merge remote-tracking branch 'origin/main' into fix/2444-igmp-id-narr…
f3811ab 
…ow-to-self

# Conflicts:
#	pcapkit/protocols/internet/igmp.py
#	tests/protocols/internet/test_igmp_unit.py

Co-authored-by: JarryShaw <15666417+JarryShaw@users.noreply.github.com>
@JarryShaw
JarryShaw merged commit 61f4afd into main Nov 1, 2026
22 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Nov 1, 2026
@JarryShaw
JarryShaw deleted the fix/2444-igmp-id-narrow-to-self branch November 1, 2026 05:30
@JarryShaw JarryShaw mentioned this pull request Nov 1, 2026
Closed
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Nov 1, 2026
@JarryShaw JarryShaw mentioned this pull request Nov 1, 2026
Merged
4 of 5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

Copilot code review Copilot

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

PyPCAPKit
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JarryShaw