ICMP: embedded_chain, and document when lax=True is the right default - #132
Merged
Conversation
decode_frame(lax=True, start=IPv4) already solves #92's core problem with zero new code: an RFC-792 error message quotes only the invoking IP header plus 8 bytes of what follows, never a full TCP/UDP header, so decoding it needs the lax path, not a try/except. Verified against a real embedded-packet fixture before writing anything. embedded_chain is the pre-wired convenience next to the existing raw embedded_packet -- same shape as this codebase's other accessor-plus- typed-view pairs. Starts at IPv4 for ICMPv4, IPv6 for ICMPv6. None for the same cases embedded_packet degrades to (non-error types, an empty body). No import cycle: icmp.py importing ip.py and walk.py at module level was verified clean (neither imports icmp.py, directly or transitively) via `python -c "import netprotocols"` and two isolated import paths before committing to top-level imports over deferred ones. The remaining acceptance criterion -- the distinction documented, so nobody reaches for lax by default -- is the actual work here: the module docstring and a new README section explain that embedded_chain's lax=True is the right default only because the truncation is RFC-mandated, expected input, not a general license for lax=True on a complete frame that fails to decode. Closes#92. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SVCFe7B1U5VeJRoUbx24vb
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Sep 4, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes#92.
decode_frame(lax=True, start=IPv4)already solved this issue's core problem with zero new code — verified before writing anything: an RFC-792 ICMP error message quotes only the invoking IP header plus 8 bytes of what follows, never a full TCP/UDP header, so decoding it needs the lax path, not atry/except. Of the issue's four acceptance criteria, three were already true on master; the fourth — the distinction is documented, so nobody reaches forlaxby default — is the actual work here, plus a small pre-wired convenience accessor.What's included
ICMPv4/ICMPv6(via the shared_ICMPbase) gainembedded_chain:decode_frame(icmp.embedded_packet, lax=True, start=IPv4-or-IPv6), next to the existing rawembedded_packet— same shape as this codebase's other accessor-plus-typed-view pairs.Nonefor the same casesembedded_packetdegrades to (non-error message types, an empty body).icmp.pynow importsIPv4/IPv6(fromlayer3.ip) anddecode_frame(fromwalk) at module level. Verified no import cycle before committing to that (rather than deferring, perwith_checksums()'s pattern): neitherip.pynorwalk.pyimportsicmp.py, directly or transitively — checked viapython -c "import netprotocols"and two isolated import paths (from netprotocols.layer3.icmp import ...,from netprotocols.layer3 import icmp).embedded_chain'slax=Trueis the right default here only because the truncation is RFC-mandated, expected input — not a general license to reach forlax=Trueon a complete frame that fails to decode, which is still a bug to raise on.tests/test_icmp.py:embedded_chaindecodes a truncated embedded packet cleanly withstopped_byset (ICMPv4 →IPv4, ICMPv6 →IPv6); returnsNonefor non-error types and an empty body, mirroringembedded_packet's existing tests.CHANGELOG.mdentry under## [Unreleased].Verification
uv run --frozen ruff check .anduv run --frozen ruff format --check .are cleanuv run --frozen mypyis clean (strict)uv run --frozen pytestpasses locallyuv run --frozen python scripts/benchmark.py --check --threshold 15— within threshold (+14.0% vs. baseline)CHANGELOG.mdhas an entry under## [Unreleased]No new protocol/dispatch change, so that checklist block doesn't apply.
Notes
Last of the three remaining Tier 2 issues (#90 → #89 → #92, per the roadmap's #107 working agreement). Once this merges, Tier 2 (#104) is fully closed — the version bump to 2.0.0 and CHANGELOG heading change are a release-cut decision left to the repo owner.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SVCFe7B1U5VeJRoUbx24vb
Generated by Claude Code