Skip to content

fix(ipv6): parse unrecognised extension headers generically (#891) - #904

Merged
JarryShaw merged 1 commit into
mainfrom
fix/891-ipv6-generic-ext
Sep 29, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/891-ipv6-generic-ext

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Fixes #891: a failed or unrecognised IPv6 extension header used to collapse the whole packet to Raw (proto = info.next on a Raw fallback has no .next). Adds IPv6_GenericExt, a generic RFC 6564 §4 parser for the closed set of extension headers whose wire format is guaranteed (AH/IPv6-Frag use their own length rule). IPv6._import_next_layer substitutes it for a recognised header whose own parser raises, and Shim6 (no dedicated parser at all) now dispatches to it directly instead of defaulting to Raw. IPv6._decode_next_layer's walk now stops structurally -- on any layer whose parsed info carries no next attribute -- rather than reading info.next unconditionally: this is what fixes BIT-EMU/253/254 (no dedicated parser, so they resolve to plain Raw) without naming them, and keeps the packet's own header intact instead of losing it. ESP is unaffected either way: it has its own dedicated parser, and that parser's info always carries a next (None, since RFC 4303 encrypts the real value), so the walk already ended cleanly after it. An overrun warns (house SchemaWarning convention) then stops the walk rather than clipping, since a clipped skip distance would fabricate a layer from trailing bytes.

Breaking: protochain for an affected packet gains a real segment instead of collapsing (e.g. Ethernet:IPv6:IPv6-GenericExt:UDP instead of Ethernet:Internet_Protocol_version_6). The #889 test pinning the old collapse is re-pointed at the corrected chain.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) bug Issues reporting a defect (set by the bug report template; a default, not an assessment) breaking Breaks public-facing behaviour or API (apply alongside the type label) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

CI is red on four legs — Python 3.11, 3.12, 3.13, 3.14 — and it is essentially one defect, not four. Run 36516062044. Root-caused rather than reported as a red mark.

The dominant cause: IPv6_GenericExt is not exported from the aggregator packages. tests/project/test_public_api.py:438 fails identically on every leg:

SUBFAILED(package='pcapkit.protocols')        Lists differ: ['IPv6_GenericExt'] != []
SUBFAILED(package='pcapkit.protocols.data')   Lists differ: ['IPv6_GenericExt'] != []
SUBFAILED(package='pcapkit.protocols.schema') Lists differ: ['IPv6_GenericExt'] != []

Both test_aggregators_export_every_public_attribute and test_every_public_package_exports_every_public_attribute trip on it. The PR adds the class plus its data and schema modules and wires the three __init__.py files, but the new name is missing from what those aggregator packages re-export — so the module is importable while the public surface does not admit it exists. That is the fix: add IPv6_GenericExt to each aggregator's public surface the way its siblings are.

Second, independent: make isort is red. tests/project/test_isort_clean.py:346 — AssertionError: 1 != 0 : make isort is red on a clean checkout -- Makefile:133 wants changes. The PR's checklist ticks make isort, so either it was not run or it was run before the last edit. Run it and commit the result.

Third, and I believe pre-existing rather than caused here: two tests/project/test_module_isolation.py:223 failures, on the CLI stand-in leak and the integration-tier bootstrap leak, both reporting "the result still depends on collection order (#660)". That message is the test naming a known condition against #660, not a new regression — but confirm it fails identically on main before dismissing it, and say so either way rather than assuming.

Note the first two are genuine and would have been caught locally: the public-API sweep and make isort both run without the CI matrix. The review: pending label stays until these are fixed and the cross-review lands.

@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES on 4581fa072 — cross-review (opus; author sonnet). Five items. The first is my fault, not the author's, and it means the ruling on this issue was given on a premise I got wrong.

1. Codes 147, 253 and 254 still collapse the whole IPv6 packet. The #891 defect survives for 3 of the 12 IANA codes — and the docstrings call that benign.

PR #904 head:                                          origin/main:
  140  IPv6  Ethernet:IPv6:IPv6_GenericExt:UDP           140  Raw  Ethernet:Internet_Protocol_version_6
  147  Raw   Ethernet:Internet_Protocol_version_6        147  Raw  Ethernet:Internet_Protocol_version_6
  253  Raw   Ethernet:Internet_Protocol_version_6        253  Raw  Ethernet:Internet_Protocol_version_6
  254  Raw   Ethernet:Internet_Protocol_version_6        254  Raw  Ethernet:Internet_Protocol_version_6

Ethernet:Internet_Protocol_version_6 with payload=Raw is the #891 signature. Verified the mechanism myself: all three are Enum_ExtensionHeader members (BIT_EMU: 147, Use_for_experimentation_and_testing_253/254), so they enter the walk at ipv6.py:315 and reach proto = info.next at :338 — but they have no Internet.__proto__ entry (measured: None for all three), so they resolve to Raw, which has no next, and it raises exactly as before.

Where this came from: my exception table. I wrote "ESP, BIT_EMU, 253, 254 — terminal, no next header" and "these keep degrading to a plain Raw exactly as they do today". Correct about the wire format, wrong about the code: only ESP is implemented terminally (measured clean: Ethernet:IPv6:ESP). For 147/253/254, "as they do today" means destroys the packet — the defect this issue exists to fix. So my brief told the author to preserve the bug for three codes, and they faithfully did, then documented it as harmless at ipv6_generic_ext.py:51-54 and ipv6.py:445-446.

@JarryShaw — your ruling was given on that false premise. You wrote "ESP/BIT_EMU/253/254 terminate into a plain Raw as they do today", which was my framing. The fix is to make the walk break when the returned layer has no next, rather than leaving three codes broken, and to correct both docstrings. I am briefing that unless you say otherwise.

2. alias is not overridden, so the new class writes _genericext into the user-visible packet dict. str.lstrip strips a character set, not a prefix: 'IPv6_GenericExt'.lstrip('IPv6-') → '_GenericExt' → key _genericext. Siblings dodge this by defining alias explicitly with a hyphen — ipv6_frag.py:63 returns 'IPv6-Frag', ipv6_opts.py:231 returns 'IPv6-Opts' — which lstrip cleanly to frag/opts. Add alias -> 'IPv6-GenericExt'; the expected-chain strings move with it.

3. The registration leaks out of IPv6 and changes IPv4 parsing — a second breaking change the PR does not declare. ipv6_generic_ext.py:444 assigns into Internet.__proto__, and I confirmed IPv4.__proto__ is Internet.__proto__ is True. So an IPv4 packet with protocol byte 140 now parses as IPv4:IPv6_GenericExt:UDP:Raw where it was IPv4:Shim6 — an IPv6 extension-header class walking a next-header chain out of an IPv4 payload, and the old segment was strictly more informative. Register at the IPv6 dispatch level or gate read() on version == 6.

4. New Sphinx warning. docs/source/pcapkit/protocols/internet/ipv6_generic_ext.rst:70 — WARNING: Footnote [*] is not referenced. [ref.footnote]. The .. [*] :rfc:6564`` has no [*]_ in the body; `ipv6_frag.rst:9` shows the pattern. Note the author reported Sphinx unavailable in its venv; it is present (`sphinx 9.1.0`).

5. Data_IPv6_GenericExt.protocol is annotated non-Optional but set to None. data/internet/ipv6_generic_ext.py:31 declares protocol: 'ExtensionHeader'; the code sets it to None when no enum member matches, and the class property at :179 is correctly Optional. Make the data model Optional[ExtensionHeader].

Confirmed good, and some of it strongly. The core fix works and the new assertion is the truth rather than code-shaped — no :UDP is owed on that packet because the MH's Hdr Ext Len covers the whole payload, and a variant with real trailing UDP bytes does extend the chain. The RFC 6564 arithmetic is right for all seven headers, verified by forcing each parser to raise. Overrun is warn-then-stop with the SchemaWarning firing rather than the loop ending silently — my open question, now answered. __index__ raises for the classmethod reason and explicitly contrasts itself with Raw. All three of next/length/protocol are genuinely dynamic. Both entry paths are implemented and covered, and reverting the try/except reproduces the exact AttributeError: 'Raw' object has no attribute 'next'.

And the author was right to push back on my Shim6 instruction. I told it to remove Shim6 from tables and docstrings; it kept the wire-format row and disclaimed the parser separately at :57-61. That is the accurate rendering, since IANA marks 140,Shim6,…,Y,[RFC5533] — the format conforms, the parser never existed. Keeping it as written.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
@JarryShaw
JarryShaw force-pushed the fix/891-ipv6-generic-ext branch from 4581fa0 to 9a4b0b8 Compare September 29, 2026 04:41
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Sep 29, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Both rounds fixed on 9a4b0b8e5 — including the defect my own exception table caused. Re-review dispatched rather than granting on my spot-check.

The structural fix works, and it is genuinely structural. ipv6.py:428 is if not hasattr(info, 'next'): — no code list. The three codes appear only in explanatory comments at :85, :406, :487. Measured by me in an asserted tree:

code 147 (BIT_EMU) -> protochain='IPv6:Raw:Raw'   src/dst intact=True
code 253           -> protochain='IPv6:Raw:Raw'   src/dst intact=True
code 254           -> protochain='IPv6:Raw:Raw'   src/dst intact=True

Against Ethernet:Internet_Protocol_version_6 with an AttributeError before. So the packet survives, and it survives for any future unimplemented IANA code rather than for three named ones — which is the right shape, since a code table is precisely what went wrong when I wrote one.

The IPv4 leak is closed. ipv6_generic_ext.py:418 gates __post_init__ on version != 6, and an IPv4 packet carrying protocol byte 140 now reads protochain='IPv4:Shim6' — exactly the pre-registration rendering, verified by me.

alias is hyphenated — :187 returns Literal["IPv6-GenericExt"], matching IPv6_Frag/IPv6_Opts.

Why a re-review rather than my sign-off. This is a 503-line new protocol class plus a change to the extension-header walk itself, fixed across two rounds of five and six items. And the author reports catching its own regression mid-fix: moving proto = info.next after the fragment-header branch broke IPv6-Frag chains with 5 test failures, fixed by reordering. That is exactly the failure class my spot-check cannot reach — a hasattr guard placed one line wrong silently truncates a chain that should continue, and a truncated chain looks like success. Verifying three codes stop correctly says nothing about whether the other seven still continue correctly.

So the re-review's first job is a chain walk per header — HOPOPT, IPv6-Route, IPv6-Opts, IPv6-Frag, MH, HIP, AH, ESP, a nested multi-header chain, and one ending in a real transport layer. Also on its list: whether both GenericExt spellings are now correct in the right places (hyphenated for chain segments, underscored for the class — both legitimate, which is where a stale one hides), and whether the test_module_isolation.py "#660" failures really were downstream of the missing aggregator exports rather than a collection-order regression, since "it was someone else's bug" is the convenient conclusion.

Label moved to review: pending on this head — the previous verdict was on 4581fa072 and a verdict pinned to a superseded head is worse than none.

Also worth recording: the author corrected its own earlier diffstat, which I had to publicly flag as unreliable. It now reports ipv6.py +122/-4, test_mh_unit.py +34/-22, and the new files at +503/+45/+42/+368/+70. The reviewer will re-derive those rather than take them.

@JarryShaw

Copy link
Copy Markdown
Owner Author

CI is red on three legs (Python 3.10, 3.11, 3.13; run 36522752427) and it is one defect, not three — tests/protocols/test_dispatch_registry_unit.py pins the dispatch registry's shape and this PR deliberately changes it. Four assertions, same root cause:

test_cases_cover_every_table_named_in_the_issue   AssertionError: 39 != 38
test_every_case_has_a_pinned_target               Lists differ: ['internet/Shim6'] != []
test_registry_currently_matches_pinned_target     internet/Shim6 resolves to IPv6_GenericExt,
                                                  PINNED_TARGETS expects None
test_dispatch_reaches_target_or_is_a_recorded_degrade
                                                  internet/Shim6 no longer dispatches to None

The test is behaving correctly and its own message says what to do. From :204: "Either the registry regressed, or PINNED_TARGETS is stale and needs updating to match a deliberate change." This is the second case — registering protocol 140 → IPv6_GenericExt is the point of the change and the maintainer ruled it explicitly, so the pins are what move, not the code.

What needs updating in tests/protocols/test_dispatch_registry_unit.py:

  • the expected entry count across the seven __proto__ tables, 38 → 39 (:222);
  • PINNED_TARGETS for internet/Shim6, None → IPv6_GenericExt (:204);
  • internet/Shim6 removed from whatever currently lets it be a missing pin (:178);
  • and KNOWN_DEGRADED — Shim6 is no longer degraded, it now parses generically, so if it is listed there it comes out. Note the test's other instruction, which still binds: "If this is a newly found defect, add it to KNOWN_DEGRADED with the file:line that causes it — do not change the case to avoid it." That is the opposite direction from this change and must not be used as cover for it.

Update the pins with a comment naming this issue, so the next reader sees a deliberate change rather than a loosened assertion. A bare 38 → 39 with no reason is indistinguishable from someone making a red test green.

This is a genuinely good test and worth saying so: it detected a registry shape change from a PR that touches neither the registry file nor the test, and refused to pass silently. Exactly the class of drift that #899's census break and #909's index duplication were both about.

Sent to the author. The re-review in flight has been told, since its chain-walk task and this overlap — both turn on what protocol 140 now resolves to.

@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES on 9a4b0b8e5 — re-review (opus; author sonnet). The chain walk is clean: 19 chains, zero regressions. Two defects remain, both introduced here, both cheap.

The headline first, because it is the thing I could not check myself. Every chain that worked on origin/main is byte-identical on this head, and every chain that crashed now yields a sensible one:

HOPOPT / IPv6-Route / IPv6-Opts / MH / AH / ESP / bare UDP        unchanged
IPv6-Frag -> UDP                                                 unchanged  <- the 5-failure case
HOPOPT->Route->Opts->UDP,  HOPOPT->Route->Frag->UDP,  Frag->Opts->UDP   unchanged
HIP -> UDP                        RAISED  ->  IPv6:IPv6-GenericExt:UDP:Raw
HOPOPT->AH->MH->HIP->UDP          RAISED  ->  IPv6:HOPOPT:AH:MH:IPv6-GenericExt:UDP:Raw
Shim6 -> UDP,  HOPOPT->Shim6->UDP RAISED  ->  chain continues to transport
147 / 253 / 254                   RAISED  ->  IPv6:HOPOPT:Raw:Raw
HOPOPT->truncated Route           RAISED  ->  stops, SchemaWarning fires

So the hasattr guard truncates nothing, and its ordering relative to the Frag branch is right for all of them — Frag mid-chain and Frag followed by a further extension header both unchanged, which are the cases a misplaced guard would break.

And the fix is bigger than I framed it. I described it as three codes plus the IPv4 leak. Shim6 and any HIP whose own parser raises were also collapsing the entire packet on origin/main, and are also fixed. My framing understated it.

D1 — dead code at ipv6.py:441. proto = info.next is assigned at :432 and again at :441. Verified: the only statement between them is the if ex_proto == Enum_ExtensionHeader.IPv6_Frag: block, which breaks, so the second assignment is unreachable-in-effect. It is the leftover from the very reorder that fixed the Frag failures, and it contradicts the 13-line comment at :415-427 explaining the single required ordering. Pylint does not flag redundant reassignment, which is why it survived.

D2 — the design prose is factually wrong about ESP, in four places, and the error invites a dangerous change. Verified myself:

ESP class          : <class 'pcapkit.protocols.internet.esp.ESP'>
IPv6.__proto__[50] : ModuleDescriptor(module='pcapkit.protocols.internet.esp', name='ESP')

ESP has a dedicated, registered parser at esp.py:965, and its info does carry next (= None), so the new hasattr guard never fires for it — the walk terminates one iteration later exactly as before. But the prose says "none of the four has a next header field to read at all" and "none of which has a dedicated parser to raise from in the first place". Both false for ESP.

Why this blocks rather than being a nit: excluding ESP from __generic_ext_codes__ is correct — encrypted payload, different length rule — but the stated reason is false in a way that invites a specific wrong edit. A maintainer reading "ESP has no dedicated parser to raise from" could reasonably add ESP to the set, which would substitute IPv6_GenericExt whenever the real ESP parser raises — walking ciphertext as an extension-header chain. The correct reason is "ESP's next is always None and its payload is encrypted", not "it has no parser". Four locations: ipv6.py's __generic_ext_codes__ docstring and _import_next_layer Notes, ipv6_generic_ext.py:51, and docs/source/pcapkit/protocols/internet/ipv6_generic_ext.rst:25 — plus the PR body.

Also stale: the PR body still writes Ethernet:IPv6:IPv6_GenericExt:UDP with an underscore in a chain-segment context. The code emits IPv6-GenericExt; the tests were all fixed, the description was not.

Verified clean: the version != 6 gate is at the single construction funnel before _extf is set, no ProtocolError escapes, and an IPv4 packet with protocol 140 yields a genuine Raw with all 24 payload bytes preserved and the reason recorded in info.error. Both GenericExt spellings are correct everywhere — hyphenated only as a chain segment, underscored only as the class, zero stale. Diffstat exact. 260 tests / 0 failures in tests/protocols/internet/, 17 methods / 448 subtests across the three tests/project/ files, all under plain unittest. mypy: 5 findings, all pre-existing, identical messages at +43/+83/+107 line offsets; 321 errors on both trees, zero from the new modules. Sphinx build succeeded, 58 warnings, zero mentioning generic_ext.

The #660 question is answered properly rather than accepted: test_module_isolation.py is 6/6 green on origin/main, and every one of its subprocess selections embeds test_public_api (lines 105, 113, 120, 137, 142). So the failures were genuinely downstream of the missing exports. And the exports are in seven __init__.py files, not the four I said, each placed after IPv6_Frag and alphabetically correct.

One correction to my own brief worth flagging publicly, since this author's self-reported numbers have been quoted before: its claim of "3 pylint findings pre-existing with shifted line numbers" does not reproduce. make pylint's own flags give 39 findings on both trees, all R0401 cyclic-import at a single location with no line numbers, score 8.67/10 either way. No new pylint problem is introduced — but the figure was wrong.

Sent to the author together with the CI fix.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
- Added IPv6_GenericExt, a generic RFC 6564 §4 parser for IPv6 extension
  headers: it reads the guaranteed next-header/Hdr-Ext-Len octets and
  derives next/length/protocol from the actual parse, applying AH's and
  IPv6-Frag's own length rules where they diverge. Gated to version==6,
  since it is registered into the Internet-wide dispatch table for Shim6
  and would otherwise also activate for an IPv4 payload carrying protocol
  byte 140.
- IPv6._decode_next_layer now stops its walk structurally -- on any layer
  whose parsed info carries no `next` field -- instead of reading
  `info.next` unconditionally and crashing the whole packet when it hits
  ESP, BIT-EMU, 253, 254, or any other IANA code with no dedicated parser
  (the actual #891 defect). IPv6._import_next_layer substitutes
  IPv6_GenericExt for a recognised header whose own parser raises.
- IPv6_GenericExt.alias is hyphenated ('IPv6-GenericExt'), matching its
  siblings, so IPv6's `alias.lstrip('IPv6-')` keys the packet dict as
  'genericext' rather than '_genericext'.
- An overrun warns in the house SchemaWarning convention, then stops the
  walk rather than clipping: the length also decides where the next
  header starts, so a clipped skip distance would fabricate a layer.
- Exported IPv6_GenericExt from the pcapkit, pcapkit.protocols,
  pcapkit.protocols.data and pcapkit.protocols.schema aggregators.
- Re-pointed the #889 test pinning the pre-fix whole-packet collapse, and
  added coverage for both dispatch paths, the AH/Frag rules, the overrun
  stop, the version gate, and an unimplemented terminal code (BIT-EMU).

Build/test: tests/protocols/internet/ (260 passed, 868 subtests) and
tests/project/{test_public_api,test_isort_clean,test_module_isolation}.py
via the worktree's own pcapkit; coverage on the touched files rose from
81% to 94% with the new tests added. `make isort` clean; the two
remaining mypy findings and the pylint findings pre-exist on main.
@JarryShaw
JarryShaw force-pushed the fix/891-ipv6-generic-ext branch from 978b993 to de7177f Compare September 29, 2026 05:20
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Sep 29, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on de7177fde. All three items from the last round fixed, verified by me, and CI is now fully green (ok=62, fail=0, inc=0).

The defect my own exception table caused is fixed structurally, and I measured it rather than reading the diff. ipv6.py:428 is if not hasattr(info, 'next'): — no code list; 147/253/254 appear only in explanatory comments:

code 147 (BIT_EMU) -> protochain='IPv6:Raw:Raw'   src/dst intact=True
code 253           -> protochain='IPv6:Raw:Raw'   src/dst intact=True
code 254           -> protochain='IPv6:Raw:Raw'   src/dst intact=True
IPv4 + proto 140   -> protochain='IPv4:Shim6'     (the pre-registration rendering)

Against Ethernet:Internet_Protocol_version_6 plus an AttributeError before. It now holds for any future unimplemented IANA code rather than three named ones, which is the right shape — a code table is exactly what went wrong when I wrote one.

The three round-three items, each checked:

  • D1 — one live proto = info.next remains, at :450. The other two occurrences are comments referencing it; main has one. So the redundant assignment is genuinely gone.
  • D2 — ESP is out of the "no dedicated parser" list, which now correctly names only BIT-EMU, 253, 254, and splits ESP off with its real reason (dedicated parser, next always None because RFC 4303 encrypts it). Four locations plus the PR body.
  • Dispatch pins — PINNED_TARGETS['internet/Shim6'], the 38 → 39 and by_family['internet'] 16 → 17 counts, each carrying a #904 comment with the reason. And it verified the delta is exactly one entry by diffing cases() label sets against a scratch origin/main worktree rather than trusting the arithmetic — which is what I asked for and is the difference between a fix and a silenced test.

It also found something my brief did not anticipate, and this is the part worth recording. Pinning the registry target made probe() actually call _internet_build(140), which had no payload builder and would have raised LookupError. So it added a 13-line Shim6 builder to examples/generators/dispatch.py — outside its original scope, necessary, and commented with the reason. Pinning a dispatch target is not just a data edit; it activates a code path.

KNOWN_DEGRADED correctly needed no change: Shim6 was never listed there, being a new case rather than a previously-degraded one, and it now dispatches successfully.

Carried forward from the previous review, which remains the strongest evidence here: the chain walk across 19 chains showed every chain that worked on main is byte-identical and every one that crashed now yields a sensible chain — including IPv6-Frag mid-chain and Frag followed by a further extension header, the cases a misplaced hasattr guard would break. Rebase onto current main was clean.

Ready for you to merge. I am not merging.

@JarryShaw JarryShaw added 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 labels Sep 29, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Wait, we decided the name to be IPv6_Ext and used as a base class for all IPv6 extension headers. See #891 for the full conversion and decision.

@JarryShaw

Copy link
Copy Markdown
Owner Author

You are right about the decision, and #904 is deliberately not it — by a split recorded on #891 and never contested. Quoting that thread:

Name clash to settle: IPv6_Ext collides with the in-flight ipv6_generic_ext.py … Suggest IPv6_Ext for the base and keeping IPv6_GenericExt for the concrete generic parser — base names the family, subclass names the mechanism.

and, in the same thread's last planning comment:

#891 itself stays the narrow fix (the generic parser so the chain survives), and the IPv6_Ext base remains a separate follow-up filed after #891 lands.

So both names were meant to coexist: IPv6_Ext is the base for all 8 extension headers — the 7 that implement the contract today plus ESP, via ESP(IPsec, IPv6_Ext) mirroring AH(IPsec, IPv6_Ext), which was your own ruling — and IPv6_GenericExt is the concrete RFC 6564 parser that closes #891's chain collapse.

Measured: #904 matches its siblings exactly and introduces no inconsistency. IPv6_GenericExt(Internet[...]) is the same shape as IPv6_Route, IPv6_Frag, IPv6_Opts, HIP and MH, all of which subclass Internet[...] directly; AH subclasses IPsec[...]. None of them subclasses an IPv6_Ext today, because it does not exist yet — and I confirmed the follow-up issue has never been filed, in any state.

My recommendation: merge #904 as it stands, and I file the IPv6_Ext base issue the moment it lands. The reasoning recorded on #891 still holds: #904 creates the 8th member, so designing the base against 7 and retrofitting the 8th is worse than letting the 8th exist first — and the proof obligations are opposite, since #891 must ship a test that fails without it while the refactor's argument is "the diff only moves lines and the suite is unchanged". Merging them would make that second argument unavailable.

Filing it also unblocks #895, which is blocked precisely on the IPv6_Ext base existing.

If you would rather have the base first, say so and I will hold #904 and dispatch the refactor instead — but then IPv6_GenericExt lands as a subclass afterwards rather than #904 shipping now. Marking needs: decision; the GOOD TO GO verdict on de7177fde stands either way, since this is a sequencing question and not a defect.

@JarryShaw JarryShaw added the needs: decision Waiting on the maintainer to decide — not blocked by other work label Sep 29, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

My initial thought was to directly rename IPv6_GenericExt to IPv6_Ext, and use it as the base class and the unknown extension header's fallback parser.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Measured your proposal, and the obstacle I expected is not there — one class can do both jobs. Recording the mistake first: I started from "its __index__ raises UnsupportedCall, so it cannot be a base for six numbered headers," and a grep suggested none of the six defined __index__ themselves. That grep was too narrow. Measured in-process instead:

IPv6_Route   __index__() = <TransType.IPv6_Route: 43>        defined on IPv6_Route
IPv6_Frag    __index__() = <TransType.IPv6_Frag: 44>         defined on IPv6_Frag
IPv6_Opts    __index__() = <TransType.IPv6_Opts: 60>         defined on IPv6_Opts
HIP          __index__() = <TransType.HIP: 139>              defined on HIP
MH           __index__() = <TransType.Mobility_Header: 135>  defined on MH
AH           __index__() = <TransType.AH: 51>                defined on AH

Every one defines it on itself, so a raising __index__ on the base is shadowed and harms nothing. Same for name and alias — each header has its own Literal. And the class already carries the extension-header contract the base is for: its payload and protochain return ... | NoReturn, the _extf guard shape that AH has at ah.py:75,87,99.

So the real cost is narrower than a design clash: it is the loss of a safety net for anything added later. A future extension header that forgets to override inherits concrete values and silently reports itself as "Generic IPv6 Extension Header" with alias IPv6-GenericExt, and __index__ raising instead of returning its number. Today's six are fine because all six shadow everything; nothing enforces that the seventh will.

My recommendation: do it your way — rename to IPv6_Ext, one class, base and fallback — and buy the safety net back with a test asserting every IPv6_Ext subclass overrides name, alias and __index__, and that __index__ returns a real TransType. That converts an invariant currently held by luck into one held by CI, and it is cheaper than maintaining an abstract base plus a concrete subclass.

Where: a follow-up, not this PR. #904 is reviewed, verified and mergeable at de7177fde; the rename touches all six siblings plus ESP(IPsec, IPv6_Ext) per your earlier ruling, and folding that in would discard the cross-review and the chain-walk evidence behind it. Merge #904, and I file the rename-plus-reparent immediately — which also unblocks #895.

Still needs: decision, since "my initial thought was" is not yet an instruction. Say go and I dispatch it.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Go

@JarryShaw

Copy link
Copy Markdown
Owner Author

Ruling recorded, verbatim:

Go

Taken as approving the plan in my last comment: merge #904 as it stands, and the rename lands as its own change rather than being folded in here. Filed as #917, labelled refactor, breaking, blocked.

#917 carries the whole design so none of it is rediscovered: the rename to IPv6_Ext with the file moving to ipv6_ext.py, all 8 members re-parented including AH(IPsec, IPv6_Ext) and ESP(IPsec, IPv6_Ext) per your #891 ruling, the measurement showing one class can hold both roles because every known header shadows __index__/name/alias, and — required, not optional — the test asserting every subclass overrides those three and that __index__ returns a real Enum_TransType. That test is what replaces the safety net the merge removes.

It is blocked on this PR, because #904 adds the very file #917 renames and holds esp.py and mh.py besides. Checkable:

gh pr view 904 -R JarryShaw/PyPCAPKit --json state -q .state   # MERGED

Removing needs: decision. This PR is unchanged at de7177fde, still review: good-to-go, 62/62 green — ready for you. Landing it also unblocks #895, which names the IPv6_Ext base as its blocker.

@JarryShaw JarryShaw removed the needs: decision Waiting on the maintainer to decide — not blocked by other work label Sep 29, 2026
@JarryShaw
JarryShaw merged commit b3551cb into main Sep 29, 2026
63 checks passed
@JarryShaw
JarryShaw deleted the fix/891-ipv6-generic-ext branch September 29, 2026 13:30
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 29, 2026
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…up (#877)

Phase 2 of #877, the 11 classes across 8 files not held by #913/#904:
TransportProtocol, FinalisedState, Completion, ftp.Type, httpv1.Type,
Criticality, PDUKind, PacketDirection, PacketReception, WireGuardKeyLabel,
and FrameType.Flags (carrying its 6 per-frame subclasses transitively).
Each now mixes in EnumLookup ahead of its enum base for the shared
get/get_all contract.

TransportProtocol and Criticality already had their own get, both as a
staticmethod against EnumLookup.get's classmethod (the #908/#915 trap).
Both are now classmethods delegating to super().get(), keeping only what
the base does not reproduce -- TransportProtocol's case-fold and no-mint
refusal, Criticality's case-sensitive miss -- each re-raised as the
ValueError callers already depend on rather than the base's KeyError.
Each gained a default parameter forwarded to the base, since dropping one
the base declares is a real classmethod-override violation under mypy.

Updated test_const_enum_get.py's exclusion set for TransportProtocol's
new default, and added test_enum_lookup_reparent_877_unit.py pinning the
re-parenting, both preserved overrides, and no member-table growth.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…up (#877)

Phase 2 of #877, the 11 classes across 8 files not held by #913/#904:
TransportProtocol, FinalisedState, Completion, ftp.Type, httpv1.Type,
Criticality, PDUKind, PacketDirection, PacketReception, WireGuardKeyLabel,
and FrameType.Flags (carrying its 6 per-frame subclasses transitively).
Each now mixes in EnumLookup ahead of its enum base for the shared
get/get_all contract.

TransportProtocol and Criticality already had their own get, both as a
staticmethod against EnumLookup.get's classmethod (the #908/#915 trap).
Both are now classmethods delegating to super().get(), keeping only what
the base does not reproduce -- TransportProtocol's case-fold and no-mint
refusal, Criticality's case-sensitive miss -- each re-raised as the
ValueError callers already depend on rather than the base's KeyError.
Each gained a default parameter forwarded to the base, since dropping one
the base declares is a real classmethod-override violation under mypy.

Updated test_const_enum_get.py's exclusion set for TransportProtocol's
new default, and added test_enum_lookup_reparent_877_unit.py pinning the
re-parenting, both preserved overrides, and no member-table growth.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…up (#877) (#921)

Phase 2 of #877, the 11 classes across 8 files not held by #913/#904:
TransportProtocol, FinalisedState, Completion, ftp.Type, httpv1.Type,
Criticality, PDUKind, PacketDirection, PacketReception, WireGuardKeyLabel,
and FrameType.Flags (carrying its 6 per-frame subclasses transitively).
Each now mixes in EnumLookup ahead of its enum base for the shared
get/get_all contract.

TransportProtocol and Criticality already had their own get, both as a
staticmethod against EnumLookup.get's classmethod (the #908/#915 trap).
Both are now classmethods delegating to super().get(), keeping only what
the base does not reproduce -- TransportProtocol's case-fold and no-mint
refusal, Criticality's case-sensitive miss -- each re-raised as the
ValueError callers already depend on rather than the base's KeyError.
Each gained a default parameter forwarded to the base, since dropping one
the base declares is a real classmethod-override violation under mypy.

Updated test_const_enum_get.py's exclusion set for TransportProtocol's
new default, and added test_enum_lookup_reparent_877_unit.py pinning the
re-parenting, both preserved overrides, and no member-table growth.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
… onto EnumLookup (#930)

Finishes #877's phase 2, which #921 left seven classes out of because their
files were held by #913/#904 at the time: CommandType and
ConformanceRequirement (const/ftp/command.py), ESPStatus
(protocols/internet/esp.py), and FastBindingAcknowledgmentStatus,
IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus
(protocols/internet/mh.py). Both blockers have since merged. Each now mixes
in EnumLookup ahead of its enum base; member-table sizes are unchanged.

FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own
get, still a staticmethod that never calls super() -- left untouched, since
#923's EnumKeyError name-miss conversion already matches the base's shape.
mypy's [override] and pylint's arguments-differ against the kept decorator
are suppressed rather than resolved by widening it. The other five are
pure re-parents.

Brought conventions.rst and its own doc-claims test in line with #929,
which merged in the interim: phase 2 is now 24 of 24, zero enumerations
outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning
the re-parenting, the kept overrides, no growth, and the zero-outside
census; fixed three tests whose claims this change made stale
(test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit).

Build: mypy/pylint/isort clean against baseline; affected test files pass.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
… onto EnumLookup (#930)

Finishes #877's phase 2, which #921 left seven classes out of because their
files were held by #913/#904 at the time: CommandType and
ConformanceRequirement (const/ftp/command.py), ESPStatus
(protocols/internet/esp.py), and FastBindingAcknowledgmentStatus,
IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus
(protocols/internet/mh.py). Both blockers have since merged. Each now mixes
in EnumLookup ahead of its enum base; member-table sizes are unchanged.

FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own
get, still a staticmethod that never calls super() -- left untouched, since
#923's EnumKeyError name-miss conversion already matches the base's shape.
mypy's [override] and pylint's arguments-differ against the kept decorator
are suppressed rather than resolved by widening it. The other five are
pure re-parents.

Brought conventions.rst and its own doc-claims test in line with #929,
which merged in the interim: phase 2 is now 24 of 24, zero enumerations
outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning
the re-parenting, the kept overrides, no growth, and the zero-outside
census; fixed three tests whose claims this change made stale
(test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit).

Build: mypy/pylint/isort clean against baseline; affected test files pass.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
… onto EnumLookup (#930)

Finishes #877's phase 2, which #921 left seven classes out of because their
files were held by #913/#904 at the time: CommandType and
ConformanceRequirement (const/ftp/command.py), ESPStatus
(protocols/internet/esp.py), and FastBindingAcknowledgmentStatus,
IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus
(protocols/internet/mh.py). Both blockers have since merged. Each now mixes
in EnumLookup ahead of its enum base; member-table sizes are unchanged.

FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own
get, still a staticmethod that never calls super() -- left untouched, since
#923's EnumKeyError name-miss conversion already matches the base's shape.
mypy's [override] and pylint's arguments-differ against the kept decorator
are suppressed rather than resolved by widening it. The other five are
pure re-parents.

Brought conventions.rst and its own doc-claims test in line with #929,
which merged in the interim: phase 2 is now 24 of 24, zero enumerations
outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning
the re-parenting, the kept overrides, no growth, and the zero-outside
census; fixed three tests whose claims this change made stale
(test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit).

Build: mypy/pylint/isort clean against baseline; affected test files pass.
JarryShaw added a commit that referenced this pull request Sep 30, 2026
… onto EnumLookup (#932)

Finishes #877's phase 2, which #921 left seven classes out of because their
files were held by #913/#904 at the time: CommandType and
ConformanceRequirement (const/ftp/command.py), ESPStatus
(protocols/internet/esp.py), and FastBindingAcknowledgmentStatus,
IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus
(protocols/internet/mh.py). Both blockers have since merged. Each now mixes
in EnumLookup ahead of its enum base; member-table sizes are unchanged.

FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own
get, still a staticmethod that never calls super() -- left untouched, since
#923's EnumKeyError name-miss conversion already matches the base's shape.
mypy's [override] and pylint's arguments-differ against the kept decorator
are suppressed rather than resolved by widening it. The other five are
pure re-parents.

Brought conventions.rst and its own doc-claims test in line with #929,
which merged in the interim: phase 2 is now 24 of 24, zero enumerations
outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning
the re-parenting, the kept overrides, no growth, and the zero-outside
census; fixed three tests whose claims this change made stale
(test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit).

Build: mypy/pylint/isort clean against baseline; affected test files pass.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(ipv6): parse unrecognised extension headers generically so the chain survives

1 participant