Skip to content

refactor(schema): share the IPv6-option and byte-order schema helpers (#1519) - #1528

Merged
JarryShaw merged 1 commit into
mainfrom
refactor/1519-shared-schema-helpers
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
refactor/1519-shared-schema-helpers

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

  • IPv6 option helpers: the nine HOPOPT / IPv6-Opts helpers now live once, in the new schema/internet/ipv6_option.py. The AST diff found no difference beyond the error prefix and the module-local classes. The helpers take prefix=, and the selectors also take base=/unassigned=. hopopt.py and ipv6_opts.py bind them in functions with the same names, and import rpl_opt_sub_tlv_len and pad_opt_data_len unchanged. The smf_i_dpd_tid_selector docstring had the TaggerID values off by one (IPv4 is 2, not 1), and this fixes it.
  • Byte-order helper: byteorder_callback and packet_byteorder now live in the new schema/misc/byteorder.py, and both pcap/frame.py and pcapng.py use them. They take the PCAP-NG body, a strict superset: it also falls back to an enclosing __packet__. For Frame the answer is unchanged on every packet the library passes in: all 17,940 Frame calls on the samples carry byteorder and none carries __packet__, and the old and new bodies agree on each one.
  • Byte-identical: parse→rebuild, from_data(info.to_dict()) and make→parse on all 23 sample captures (1,495 Frame, 121 PCAPNG, 15 HOPOPT, 15 IPv6_Opts layers) give the same digest as origin/main. So do 52 hand-built option cases covering the branches and every raise, under both headers.
  • Tests: test_ipv6_option_helpers_1519_unit.py has 10 tests. They pin every helper, the exact error text under both prefixes, and the delegation; on origin/main, 8 fail and the 2 header-behaviour pins pass. test_schema_byteorder_1519_unit.py has 4 tests. They pin the helper, its __packet__ fallback, that both schemas use it, and a frame header read through an enclosing packet; all 4 fail on origin/main.
  • Harnesses: two harnesses that read callback code now also follow a callback into the shared helper: tests/corekit/test_fields_misc.py for switch discovery and test_schema_roundtrip_unit.py for driver discovery. Without that, the round-trip harness lost the len driver for CALIPSO, MPL and SMF I-DPD. Its driver census now matches origin/main on all 447 schema classes.
  • mypy: whole-package make mypy gives 332 errors in 42 files, the same error set as main, and this PR adds no type: ignore. The selectors type base as EnumMeta, and annotate the registry lookup Optional[Type[Schema]], because the registries are defaultdict(lambda: None).
  • Docs: there are new internet/ipv6_option.rst and misc/byteorder.rst pages, and the HOPOPT, IPv6-Opts and PCAP-NG pages point to them.

Refs #1519

@JarryShaw JarryShaw added refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the refactor/1519-shared-schema-helpers branch from f4e32c8 to e64f991 Compare October 9, 2026 22:48
@JarryShaw JarryShaw changed the title refactor(schema): share the IPv6-option and byte-order schema helpers (#1519) refactor(schema): share the IPv6-option schema helpers (#1519) Oct 9, 2026
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the refactor/1519-shared-schema-helpers branch from e64f991 to 8cab590 Compare October 9, 2026 23:19
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 90.08% (unit tier, Python 3.14, 31a76d164, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 842 2356 756 92.09%
pcapkit/corekit 2202 67 710 34 96.26%
pcapkit/dumpkit 258 2 90 2 98.85%
pcapkit/foundation 2650 98 934 42 94.98%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16982 229 4540 189 98.03%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 8cab59044. Cross-reviewed on Sonnet; the author ran Opus. CI is green and behaviour is byte-identical, but two things need fixing.

Confirmed:

  • All 9 shared helpers match the originals across 1,186 inputs under both prefixes, including return values, exception types and exact messages. Every branch is covered.
  • All 30 sample HOPOPT/IPv6-Opts layers, and 1,862 hand-built blobs, give identical output.
  • The test_schema_roundtrip_unit.py harness fix is correct. Driver sets for all 447 schema classes match main, and the new branch fires exactly 14 times (once per wrapper), adding no spurious drivers.
  • test_fields_misc.py still discovers 10 switches. Fail-before is 8.

To fix:

  1. make mypy gains 2 errors in the new module (ipv6_option.py:141, :212: Callable[...] is not indexable). base: Type[EnumSchema] makes mypy read .registry as the instance property. Use a narrower annotation or a targeted ignore.
  2. byteorder_callback: the owner's refactor: move shared helper functions into utilities or corekit by use case #1519 ruling was to share it. The PCAP-NG body is a strict superset: it falls back to __packet__. All 17,940 Frame calls on the samples have byteorder set, and with the PCAP-NG-style body shared, tests/protocols/misc and the round trips pass, apart from the new pin test. Please share it per the ruling, and drop that pin.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@JarryShaw
JarryShaw force-pushed the refactor/1519-shared-schema-helpers branch from 8cab590 to 88f93e4 Compare October 10, 2026 00:42
@JarryShaw JarryShaw changed the title refactor(schema): share the IPv6-option schema helpers (#1519) refactor(schema): share the IPv6-option and byte-order schema helpers (#1519) Oct 10, 2026
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 88f93e41f. Sonnet delta review; the author ran Opus. Both round-1 items are fixed, and behaviour is unchanged. Four citations need fixing, because this PR moved the functions they point at.

Confirmed:

  • mypy: whole-package make mypy gives 332 errors on main and 332 here. The only moved line is the existing unused-ignore.
  • byteorder_callback is shared: the Frame and PCAP-NG schemas call the same function, and pcapng re-exports it.
  • Frame path: all 5,876 callback calls on the 16 .pcap samples carry byteorder, and the old and new bodies agree on every one. A SHA-256 over to_dict() is identical on main and here for 1,590 frames from 22 captures.
  • Fail-before: all 4 new tests fail on main.

To fix:

  1. pcapkit/corekit/fields/misc.py:570 cites pcapkit.protocols.schema.misc.pcapng.packet_byteorder. That is now a re-export, so point it at pcapkit.protocols.schema.misc.byteorder.packet_byteorder. On main this reference never resolved; the new byteorder.rst makes it resolve.
  2. Three docstrings still say byteorder_callback lives in pcapkit/protocols/schema/misc/pcap/frame.py: tests/protocols/misc/pcap/test_header_frame_unit.py:450, tests/protocols/misc/pcap/test_frame_endian_runtime.py:15 and examples/generators/endian.py:15. Name the new module, or say "then lived in" for the history of frame.py:199 seeds packet['bytesorder'] where readers expect 'byteorder', and no big-endian PCAP fixture exists to catch it #605.

Optional: the new # type: ignore[unreachable] (ipv6_option.py:218) can go if you annotate the lookup schema: 'Optional[Type[Schema]]' = base.registry[func]. The reviewer measured a clean mypy run with that change.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
…#1519)

- Move the nine HOPOPT / IPv6-Opts option helpers into a new
  `pcapkit.protocols.schema.internet.ipv6_option`, parameterised by the
  error prefix and, for the selectors, by the option schemas to return.
  `hopopt.py` and `ipv6_opts.py` bind them in same-named functions, and
  use `rpl_opt_sub_tlv_len` and `pad_opt_data_len` unchanged.
- Correct the TaggerID values in the `smf_i_dpd_tid_selector` docstring
  (IPv4 is 2 and IPv6 is 3, not 1 and 2).
- Move `byteorder_callback` and `packet_byteorder` into a new
  `pcapkit.protocols.schema.misc.byteorder`, with the PCAP-NG body, and
  use it from both the PCAP frame header and PCAP-NG.
- Let the callback introspection in `tests/corekit/test_fields_misc.py`
  (address-typed switches) and `tests/corekit/test_schema_roundtrip_unit.py`
  (driver fields) read the shared helper a callback delegates to.
- Document both shared modules on new docs pages, and point the HOPOPT,
  IPv6-Opts and PCAP-NG pages at them.

Byte-identical on every sample capture; new tests pin each helper, both
error prefixes and the `__packet__` fallback.
@JarryShaw
JarryShaw force-pushed the refactor/1519-shared-schema-helpers branch from 88f93e4 to 31a76d1 Compare October 10, 2026 01:02
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 31a76d164. Sonnet delta review; the author ran Opus. The delta from 88f93e41f is the 5 files requested (+15/−14), and nothing else moved.

  • Citations: corekit/fields/misc.py:570 now targets pcapkit.protocols.schema.misc.byteorder.packet_byteorder, which exists at byteorder.py:25. The three frame.py:199 seeds packet['bytesorder'] where readers expect 'byteorder', and no big-endian PCAP fixture exists to catch it #605 docstrings now say "then in pcap/frame.py, now in schema/misc/byteorder.py". A sweep of pcapkit/, docs/, tests/ and examples/ (120 hits, with a 2-line window) finds no other citation of the old definition site.
  • mypy: whole-package with the Makefile flags gives 332 errors in 42 files on both main and this head. The two error sets are identical, compared by file and message with line numbers stripped; I re-compared them myself. The PR adds no type: ignore lines, and the new modules have 0 errors.
  • The Optional[Type[Schema]] locals: they are never evaluated at runtime, so Optional exists only under TYPE_CHECKING. Both selectors ran through their None branches, and test_ipv6_option_helpers_1519_unit.py gives 10 passed with 78 subtests.
  • Earlier rounds still stand: the behaviour, byte-identity and fail-before results from rounds 1 and 2 are untouched by this delta.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@JarryShaw
JarryShaw merged commit 4451381 into main Oct 10, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 10, 2026
@JarryShaw
JarryShaw deleted the refactor/1519-shared-schema-helpers branch October 10, 2026 01:52
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant