Skip to content

fix(foundation): validate a registrar's code before registering it (#2186) - #2193

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2186-registrar-validate-code
Oct 25, 2026
Merged

fix(foundation): validate a registrar's code before registering it (#2186)#2193
JarryShaw merged 1 commit into
mainfrom
fix/2186-registrar-validate-code

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description

Closes #2186.

  • Defect: the enum-keyed registrars in pcapkit/foundation/registry/protocols.py write to the registry and only then log code.name. An int, None or str code therefore raises AttributeError after the write. On main, 19 of 20 registrars left the registry changed; register_pcapng_option raises before writing, but still with AttributeError.

  • Fix: a new _check_code helper raises RegistryError unless code is an enum/aenum member. Each affected registrar calls it first: the 15 method-name ones plus register_linktype, register_pcap, register_pcapng, register_ethertype and register_transtype. Each Raises: section names every RegistryError cause it has.

  • Why reject, not accept an int:

    • no int, None or str code ever completed on main. The only non-enum keys that did were hashable duck-typed objects carrying .name/.opt_name, which nothing in the repo uses;
    • the PCAP-NG option registry keys on code.opt_name, so an int has no key there;
    • every affected registrar is annotated with an enum type.

    The registrars typed int | Enum (register_tcp, register_udp, register_sctp, register_apptype) are untouched.

  • Test: tests/foundation/registry/test_registrar_code_check_unit.py calls each registrar with 250, None and '250', and asserts RegistryError with every involved registry unchanged. On main, 60 subtests fail.

  • Tests run: tests/foundation/registry (45), the protocol registration tests (55), the docstring contract and the tier guard all pass.

  • Merge order with docs(foundation): document RegistryError on the method-name registrars (#2176) #2183: both add the same Raises: sections. This PR's sections already include docs(foundation): document RegistryError on the method-name registrars (#2176) #2183's meth wording.

@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 25, 2026
@JarryShaw

JarryShaw commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at f15ec944d (Sonnet): on main all 60 primitive-code calls crash and 19 of the 20 leave a registry entry behind. Every pcapkit.const enum passes the check, the int | Enum registrars do accept ints, and the snapshot check catches a mis-ordered _check_code. Not breaking; the body now notes the duck-typed-key exception.

@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 25, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 25, 2026
@JarryShaw JarryShaw mentioned this pull request Oct 25, 2026
Merged
5 tasks done
@github-actions

github-actions Bot commented Oct 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Coverage: 91.55% (unit tier, Python 3.15, 782957445, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19599 880 2618 866 92.12%
pcapkit/corekit 2589 75 816 37 96.48%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 2994 23 1038 25 98.81%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17428 219 4584 185 98.15%
pcapkit/toolkit 869 36 298 10 95.20%
pcapkit/utilities 504 7 146 5 98.15%
pcapkit/vendor 4432 1988 1018 167 51.87%

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

@JarryShaw

JarryShaw commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Ready to merge at f15ec944d. Merging closes #2186.

@JarryShaw
fix(foundation): validate a registrar's code before registering it (#…
7829574 
…2186)

Problem: the enum-keyed registrars register first and then log code.name,
so an int, None or str code raised AttributeError after the protocol's
table (and schema table) had already been written.

Fix: a _check_code() helper rejects a code that is not an enum member with
RegistryError before any registry is touched. Applied to the fifteen
method-name registrars and to register_linktype, register_pcap,
register_pcapng, register_ethertype and register_transtype, which share the
pattern; Raises: sections added to each.

Test: tests/foundation/registry/test_registrar_code_check_unit.py calls
each registrar with 250, None and '250' and checks RegistryError is raised
and every registry matches its snapshot.
@JarryShaw

JarryShaw commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto main after #2183 merged: f15ec944d → 782957445.

@JarryShaw
JarryShaw force-pushed the fix/2186-registrar-validate-code branch from f15ec94 to 7829574 Compare October 25, 2026 14:46
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 25, 2026
@JarryShaw

JarryShaw commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at 782957445 (Sonnet rebase check). The rebase changed only the Raises: context: the code and the test match the old head. It removed exactly #2183's 15 two-line entries, each replaced by one entry naming the same _read_* prefix. No docstring has a duplicate section, and the registry tests and the docstring contract pass (52).

@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 25, 2026
@JarryShaw

JarryShaw commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Ready to merge at 782957445. Merging closes #2186.

  • Review: Sonnet GOOD TO GO.
  • CI: Required checks passed SUCCESS, 0 failed. Still open: the board's Sync Status and the non-gating Python graalpy-25.4 (experimental) leg.
  • KTLO: 12 legs, no warnings.
  • Ordering: foundation leg merged into main, 740 tests, 0 failures.
  • Conflicts: none with the current main.

@JarryShaw
JarryShaw merged commit 0583a96 into main Oct 25, 2026
64 checks passed
@JarryShaw
JarryShaw deleted the fix/2186-registrar-validate-code branch October 25, 2026 21:28
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 25, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 25, 2026
This was referenced Oct 25, 2026
Merged
Merged
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
@JarryShaw JarryShaw mentioned this pull request Oct 28, 2026
Merged
4 tasks done
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs(changelog): backfill entries for the #2312-#2407 range (#2409)
7f59b3e 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Two pairs share a
bullet, since each is the direct follow-up of the other: #2393 with
#2401, and #2405 with #2407. The diff on the entry file is purely
additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on the seven classes owning
a __proto__ registry (#2392), __proto_dest__ on the four enum registries
and Vendor.PROTO_DEST to generate it (#2403). Both PRs added the autodoc
entries, so both are rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs(changelog): backfill entries for the #2312-#2407 range (#2409)
813e9f8 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Three pairs share a
bullet, since each is the direct follow-up or counterpart of the other:
#2393 with #2401, #2405 with #2407, and #2392 with #2403. The diff on the
entry file is purely additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on each class owning a
__proto__ registry (#2392), __proto_dest__ on the enum registries and
Vendor.PROTO_DEST to generate it (#2403). The bullet names those classes
and registries rather than counting them, per the no-counted-numbers rule
for the unreleased entry. Both PRs added the autodoc entries, so both are
rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
JarryShaw added a commit that referenced this pull request Oct 28, 2026
@JarryShaw
docs: backfill changelog entries for the #2312-#2407 range (#2412)
27c797d 
Problem: of the 60 pull requests merged in the #2312-#2407 range, only
#2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and
#2398 and #2403 only reworded lines that were already there. Every other
user-visible change in the range -- wire-semantics fixes, maker guards,
reassembly behaviour, public-API and error-message changes -- went
unrecorded, so the entry no longer describes the release.

Fix: one entry per user-visible pull request, in each module section's
existing style and ordered by the issue it closes. Three pairs share a
bullet, since each is the direct follow-up or counterpart of the other:
#2393 with #2401, #2405 with #2407, and #2392 with #2403. The diff on the
entry file is purely additive.

Internal-only work is left out deliberately: dependency and action bumps
(#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379,
#2380, #2402), the unreachable-branch removal (#2378), and the #: comment
marker the HIP Group and Packet crawlers now write (#2381). That last one
renders nothing either way: the marker lands inside the generated
_missing_ body, conf.py's exclude-members drops _missing_ from autodoc,
and a comment in a method body is not an attribute comment to begin with.

#2392 and #2403 are recorded together, under Added rather than Fixed, for
the declarations they introduce: __proto_key__ on each class owning a
__proto__ registry (#2392), __proto_dest__ on the enum registries and
Vendor.PROTO_DEST to generate it (#2403). The bullet names those classes
and registries rather than counting them, per the no-counted-numbers rule
for the unreleased entry. Both PRs added the autodoc entries, so both are
rendered public attributes. #2392's rejection-message
change is deliberately not recorded as a fix: the bare "code must be ..."
text it prefixed came from _check_code, which #2193 introduced and which
is in no release tag -- absent at v1.5.0b14 -- so no released version ever
emitted it. #2403 is no longer skipped as internal because it reworded an
existing bullet to name __proto_dest__ while nothing credited the
attribute's introduction.

Four claims are corrected against the source rather than carried over:
FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not
1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback
read cmmd.value, so a .value-carrying object was accepted unchecked
rather than raising; #2399 left the dpkt adapter and the adapters'
ModuleNotFound raises alone, so it claims no universal prefix; and
#2397's old tail did name the version it got, so the entry quotes it and
gains the :issue:`2302` it was missing. #2370's "a Section Header Block
... still fails" sentence is dropped: #2369 settles the byte order first,
and an SHB built from raw octets with ctx=None now rebuilds byte-exact in
both byte orders.

CHANGELOG.md is regenerated with util/changelog_md.py.

Test: tests/project/ in full, 0 failures, including test_changelog_md.py's
in-step and supported-subset checks; util/changelog_md.py --check reports
the two files in step; the entry parses under docutils with no structural
warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment)

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.5

Development

Successfully merging this pull request may close these issues.

fix(foundation): a registrar given a non-enum code raises after registering it

1 participant

@JarryShaw