Skip to content

fix(corekit): resolve an unassigned enum value instead of raising aenum's ValueError (#701) - #706

Open
JarryShaw wants to merge 1 commit into
mainfrom
fix/enum-field-unassigned-value
Open

JarryShaw wants to merge 1 commit into
mainfrom
fix/enum-field-unassigned-value

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 23, 2026

Copy link
Copy Markdown
Owner

Closes #701.

The defect

EnumField.post_process resolved a wire value through its registry's constructor:

return self._namespace(value)

which raises for any value no member and no _missing_ rule accounts for. The raise is aenum's own bare ValueErrornot one of pcapkit.utilities.exceptions, so a caller cannot tell it from a bug of its own, and not an EOFError, so Extractor.record_frames does not catch it. One unassigned code therefore cost the whole extraction.

It also made the "unknown" reader every one of these formats requires unreachable. PCAP-NG declares UnknownBlock as its block-type default and carries a whole _read_block_unknown for it, but the lookup failed several frames before the dispatch that would have selected it, so the default only ever fired for the Reserved_* ranges BlockType._missing_ auto-extends. PCAP-NG repeats a block's total length at both ends precisely so a reader can skip a block type it does not recognise; that skip is what this restores.

Reproduced on 9d7890db4

The issue's repro, verbatim, with the three line numbers it cites confirmed against this branch's base (they had not gone stale): schema.py:860field.py:528numbers.py:513aenum/_enum.py:2276.

ValueError: 28 is not a valid BlockType

What changed, and what deliberately did not

An unassigned value now resolves to the same nameless pseudo-member the method already built for a field carrying no registry at all, rather than to a new value shape. That makes the diff small, and it means the dump layer already renders the result — as <unknown>::<unassigned> [28], through pcapkit/dumpkit/common.py's render_enum — and an int-keyed dispatch registry already looks it up by value like any declared member.

It is built per value rather than grafted onto the registry with aenum.extend_enum, for two measured reasons:

  • a capture carrying many distinct unassigned codes would otherwise grow a process-global registry without bound — exactly the growth ProtocolBase._lookup_registry's own docstring exists to avoid;
  • a stdlib enum.IntEnum registry is then handled identically to an aenum.IntEnum one.

No registry's own guard is touched. BlockType(28) and BlockType.get(28) raise exactly as before — the fallback lives in the field, which knows the value arrived in a fixed-width wire field. And only a foreign rejection is absorbed: a registry rejecting a value with one of pcapkit.utilities.exceptions propagates unchanged, which is what keeps this from being an unconditional except ValueError: pass.

That distinction is for registries registered from outside pcapkit.const. No generated guard raises an in-library error today and none should — tests/const/test_const_enum_builtin_parity.py::test_the_exception_is_not_an_in_library_one pins the bare, unlogged ValueError because every generated get()'s except ValueError fallback depends on it (#584, #647). Separately, no bit-flag registry is named as a plain EnumField namespace anywhere in the package, measured over all 112 call sites, so pcapkit/const/tcp/flags.py's 16-bit guard (#702) is not in this layer's path at all.

Blast radius, measured

  • 112 plain EnumField(...) call sites carry a namespace. 106 of them resolve statically to 73 distinct registries; the other 6 are alias arguments inside mh.py's br_code_selector/fb_code_selector and cannot be resolved without evaluating the call.

  • 10 of the 73 reject a probed value inside their field's own value space, and are what this changes:

    registry space first rejected a call site
    BlockType 2^32 11 pcapkit/protocols/schema/misc/pcapng.py:461
    CauseCode (SCTP) 2^16 0 pcapkit/protocols/schema/transport/sctp.py:232
    EtherType 2^16 1501 pcapkit/protocols/schema/link/arp.py:26
    FlowBindingType (MH) 2^16 256 pcapkit/protocols/schema/internet/mh.py:3079
    HITSuite (HIP) 2^8 16 pcapkit/protocols/schema/internet/hip.py:959
    MNIDSubtype (MH) 2^8 0 pcapkit/protocols/schema/internet/mh.py:722
    Option (TCP) 2^8 255 pcapkit/protocols/schema/transport/tcp.py:403
    Parameter (SCTP) 2^16 0 pcapkit/protocols/schema/transport/sctp.py:427
    Transport (HIP) 2^16 4 pcapkit/protocols/schema/internet/hip.py:1265
    UpdateNotificationReason (MH) 2^16 256 pcapkit/protocols/schema/internet/mh.py:3013
  • 63 of the 73 resolved every probed value already, and are unaffected.

  • No bit-flag registry is among the 73 — so pcapkit/const/tcp/flags.py's 16-bit guard (tests: the nameless-enum sweep probes 65536 against a 16-bit flag registry, so main fails without showing red #702) is not in this layer's path at all. TCP's flag byte is decoded bit-by-bit through BitField, never as a whole-byte enum.

  • The probe is bounded, not exhaustive — a dense band over 0..4095, every gap between declared members, and the top of the space — because a full 16-bit sweep of fifty registries mints millions of pseudo-members through extend_enum and takes over twenty minutes. So 10 is a lower bound. Two earlier passes of this census got it wrong in both directions and are worth recording: sampling only between declared members missed every registry whose lowest declared value is above zero (CauseCode, MNIDSubtype, Parameter), and using the field's byte width where the call site declares a narrower bit_length wrongly implicated PriorityLevel, whose real field is EnumField(length=1, bit_length=3) and can only ever present 0–7 — all eight declared. Verified: all 256 octets resolve to a real PriorityLevel member through that field.

  • The four post_process overrides — PortEnumField in udp.py/tcp.py/sctp.py and OptionEnumField in pcapng.py — all call super(EnumField, self).post_process(...), which the MRO sends straight to NumberField.post_process and so skips this method entirely. AppType and OptionType, whose members carry extra per-namespace attributes a value-only fallback could not supply, are reached only through those, so they are outside the blast radius by construction.

  • The isinstance(x, <registry>) chains downstream of a plain EnumField were checked one by one. The one that looked reachable — pcapkit/protocols/internet/hip.py:3799-3841's isinstance(hi_curve, Enum_ECDSACurve) / ECDSALowCurve / EdDSACurve cascade, which raise ProtocolError in its else — is not: each of those three registries mints Unassigned_%d for every value in its 2-octet width, so this fallback never fires for them and the cascade keeps seeing real members (ECDSACurve(65535)<ECDSACurve.Unassigned_65535: 65535>, isinstance True).

Why breaking

Protocol.analyze catches except Exception and downgrades the failing layer to Raw, so today an unassigned enum inside a next-layer protocol silently costs that whole layer. Measured on a one-frame PCAP whose Ethernet EtherType is 1501 — a real IANA gap:

before after
frame.protochain ETHERNET Ethernet:<unassigned>
frame.payload Raw Ethernet
eth.type (no Ethernet header parsed) <<unknown>.<unassigned>: 1501>

More information, not less — but it is a visible change in parse output for existing input, hence the label.

Evidence

tests/corekit/test_fields_numbers_unassigned_enum.py, 11 tests. 5 fail on origin/main:

$ coverage run -m pytest tests/corekit/test_fields_numbers_unassigned_enum.py
# with pcapkit/corekit/fields/numbers.py at origin/main:
5 failed, 6 passed, 1 warning, 2 subtests passed        exit 1
  test_an_unassigned_registry_value_resolves_instead_of_raising
  test_the_resolved_value_repacks_to_the_octets_it_came_from
  test_no_value_of_any_width_escapes_the_field_layer
  test_an_unassigned_block_type_no_longer_costs_the_extraction
  test_the_unknown_block_reader_is_reachable_for_an_unassigned_code

# on this branch:
11 passed, 1 warning, 2 subtests passed                 exit 0

The remaining 6 are guards that must pass in both directions — they are there to fail against a wrong fix, not against main.

The sweep is the blast radius in one line. It walks all four registries before asserting anything, so its failure names every one that regressed rather than aborting at the first: on origin/main it reports EtherType (2 octets): 62555 of 65536 and BlockType (2 octets): 65509 of 65536, while TransType and OptionNumber resolve everything in both trees and are the regression guards inside it. assertEqual(summary, []) fails on any single escape; the [:3] slice caps only how many bad values are listed, and the count in the message is the true total.

test_an_in_library_rejection_is_not_absorbed is the one that distinguishes this fix from the wrong one: against a except ValueError: pass variant of the same change it fails both subtests (SUBFAILED(namespace='StdlibBounded'), SUBFAILED(namespace='AenumBounded')), measured.

Regressions — all green, exit 0:

tests/const/  tests/protocols/schema/  tests/protocols/test_option_roundtrip_unit.py
tests/protocols/test_protocol_base_unit.py  tests/protocols/transport/test_transport_unit.py
  -> 99 passed, 1174 subtests passed, 0 SUBFAILED
tests/corekit/
  -> 190 passed, 415 subtests passed

EXPECTED_FAILURES did not move. Imported (it cannot be grepped — ** unpacking): 44 entries, unchanged. None records an enum ValueError; the only entry whose text mentions post_process is ipv4-option/QS, about pcapkit/protocols/internet/ipv4.py:1178's schema-level hook. The round-trip suite passes, so no recorded gap started passing and nothing needed deleting.

Coverage of pcapkit/corekit/fields/numbers.py, via coverage run -m pytest tests/corekit/:

stmts miss branch brpart cover
before (pre-existing tests only) 116 0 32 0 100%
after 120 0 34 0 100%

The file was already at 100%/100%, so the change is that four more statements and two more branches are covered, by eleven more tests (179 → 190) — not a percentage move.

Lint unchanged. pylint reports only the pre-existing W1309 at line 81; mypy's single [misc] finding about enum.IntEnum("<unknown>") is present verbatim on origin/main (there at line 509, here at 583); vermin reports the same Minimum required versions: 3.8.

All measurements were taken with PYTHONSAFEPATH=1, the repo venv (CPython 3.14.7), and pcapkit.__file__ asserted to this worktree.

Not done here

  • No warning is emitted on an unassigned resolution. post_process runs per field per packet, so a warning is a judgement about noise on a large capture that belongs to the owner rather than to this fix; the existing registry-less branch resolves silently too.
  • pcapkit/const/tcp/flags.py and tests/dumpkit/test_nameless_enum_rendering_unit.py are untouched — that is tests: the nameless-enum sweep probes 65536 against a 16-bit flag registry, so main fails without showing red #702, in flight separately.
  • One theoretical gap reported during review did not reproduce: the PCAP global header's network field is a plain EnumField(Enum_LinkType) parsed outside analyze()'s catch-all, but LinkType._missing_ mints Unassigned_* for every value in its width, so no link type reaches the raise. Worth recording as checked rather than left implied.

Cross-review

Reviewed by a second agent on a different model (Sonnet; this change was authored on Opus), briefed to falsify rather than to bless, running read-only with no write access to this repository or to GitHub. Verdict: good to go on the code, and needs changes on this description — it independently falsified the blast-radius numbers above, which have been corrected here and re-derived from scratch. Concretely it found: the site count was 106 rather than 112; four rejecting registries were missing (CauseCode, MNIDSubtype, FlowBindingType, UpdateNotificationReason, and a fifth, Parameter, turned up in the re-derivation); and PriorityLevel was a false positive from ignoring bit_length. It reproduced the before/after breaking table itself, confirmed the four post_process overrides are skipped by instrumenting EnumField.post_process and observing zero calls, confirmed EnumSchema.registry is a non-inserting defaultdict subclass so no dispatch registry grows, and confirmed a JSON dump of a frame carrying the pseudo-member renders as "type": "<unknown>::<unassigned> [1501]". It also noted the #648 reference overstated the connection — that issue's guard is for name is None, which a member named <unassigned> never hits — and the docstring now says so precisely. It could not sweep the full 16/32-bit space of every registry, which is why the count above is stated as a lower bound.

@JarryShaw JarryShaw added bug test Pull requests that add or correct tests (test: subject prefix) breaking Alters public API or wire output (apply alongside the type label) labels Sep 23, 2026
…um's ValueError (#701)

* `EnumField.post_process` resolved a wire value through its registry's
  constructor, so a value no member and no `_missing_` rule accounted for
  raised `aenum`'s own bare `ValueError`. That is neither one of
  `pcapkit.utilities.exceptions` nor an `EOFError`, so a caller could not tell
  it from a bug of its own and `Extractor.record_frames` did not catch it: one
  unassigned code cost the whole extraction.
* It also made the "unknown" reader every format requires unreachable. PCAP-NG
  declares `UnknownBlock` as its block-type default and carries a
  `_read_block_unknown` for it, but the lookup failed several frames before the
  dispatch that would have chosen it.
* An unassigned value now resolves to the same nameless pseudo-member the method
  already built for a field carrying no registry at all, built per value rather
  than grafted onto the registry, so no process-global registry grows.
* A rejection carrying one of `pcapkit.utilities.exceptions` still propagates, so
  a registry that deliberately bounds itself is not overruled by the field layer.
  No registry's own guard changes; `BlockType(28)` and `BlockType.get(28)` raise
  exactly as before.

Tested: 11 new tests in `tests/corekit/test_fields_numbers_unassigned_enum.py`,
5 of which fail on `origin/main`; `tests/corekit/`, `tests/const/`,
`tests/protocols/schema/` and the option round-trip suite all green.
@JarryShaw
JarryShaw force-pushed the fix/enum-field-unassigned-value branch from 9154383 to f0af610 Compare September 23, 2026 05:45
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review (Sonnet, read-only, briefed to falsify — this change was authored on Opus): good to go on the code, needs changes on the description. It falsified the blast-radius numbers — 112 call sites not 106, and 10 rejecting registries not 6, with PriorityLevel a false positive from ignoring bit_length. All corrected and re-derived; the description now carries the verified table and states the count as a lower bound. No code change was required.

@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO

Independent cross-review, read-only, on a different model from the author (author: Opus; this review: Claude Sonnet 5). Head reviewed: f0af61077. All reproductions below were run against git archive exports of 9d7890db4 (base), 9154383b0 (the head my brief was originally issued against), and f0af61077 (current head) under PYTHONSAFEPATH=1 with pcapkit.__file__ asserted to the export root each time. git diff 9154383b0 f0af61077 -- pcapkit/corekit/fields/numbers.py is a docstring-only change (no code-line diff), so every functional finding below derived on 9154383b0 was re-run and reconfirmed directly on f0af61077 (noted per item).

Note on process: partway through this review I was told #706's head had moved from 9154383b0 (what I was briefed against) to f0af61077, along with a relay of the author's own corrected blast-radius numbers. I had already independently re-derived the same corrected numbers — 112 sites, 10 rejecting registries, PriorityLevel a bit_length false-positive — before that message arrived; the match is two independent derivations, not one copying the other.

1. Fixes the reported case — CONFIRMED (re-run on f0af61077). Built the issue's exact 16-octet unknown-block-type-28 repro. On main (9d7890db4): Extractor(...) raises ValueError: 28 is not a valid BlockType from aenum/_enum.py:2276, extraction produces nothing. On the branch: no raise; PCAPNG._lookup_registry(PCAPNG.__block__, resolved) == 'unknown' and Schema_BlockType.registry.default_factory() is UnknownBlock, i.e. UnknownBlock is reachable. Ran the PR's own test file both ways: pytest tests/corekit/test_fields_numbers_unassigned_enum.py gives 5 failed, 6 passed, ... exit 1 on main and 11 passed, ... exit 0 on both 9154383b0 and f0af61077 archives.

2. Blast radius bounded and understood — CONFIRMED, with an independently-rederived correction that matches the one already in the PR body. I built an AST-based scan of every EnumField(...) call under pcapkit/protocols/schema/** resolving namespace= through each file's actual imports (not text-matching the alias), then probed each resolved registry's own constructor across its field's effective width (bit_length when present, else length*8) on the pre-fix tree. Result, derived before I saw the corrected PR body or the coordinator's relay: 112 call sites carrying namespace=, and exactly the same 10 registries reject a reachable value today — BlockType, CauseCode (SCTP), EtherType, FlowBindingType (MH), HITSuite (HIP), MNIDSubtype (MH), Option (TCP), Parameter (SCTP), Transport (HIP), UpdateNotificationReason (MH) — matching the PR's corrected table exactly, first-rejected values included. PriorityLevel is confirmed not a real gap: its only call site is vlan.py:47, EnumField(length=1, bit_length=3, namespace=Enum_PriorityLevel), and NumberField.post_process masks with self._bit_mask before EnumField.post_process ever calls the registry, so only 0–7 ever reach it and all eight are declared members (BK=0b001, BE=0b000, EE=0b010, CA=0b011, VI=0b100, VO=0b101, IC=0b110, NC=0b111) — values 8–255 do raise on PriorityLevel(v) directly, but are unreachable through the real field. Coverage of this census: exhaustive for every registry at width ≤ 12 bits (the majority); a 27%-of-space uniform sample (17,925/65,536, seeded) for all 25 sixteen-bit registries, which reproduced all 6 sixteen-bit gaps and found zero rejects among the other 19 (one full exhaustive 16-bit sweep, done to check the sampling itself, took 64s per "no-gap" registry purely from extend_enum growth and was abandoned after confirming that — this independently corroborates the PR's own "a full sweep... takes over twenty minutes" claim: my 27%-sample run of just the 25 sixteen-bit registries alone took 1346s/22.4min); light sampling only for the four 32/128-bit registries other than BlockType (ErrorCode (HTTP), SecretsType (pcapng), PayloadProtocolIdentifier (SCTP), CGAType (MH)) — none showed a reject in ~4,800 sampled values each, but I did not exhaust those and say so plainly. So: 10 holds, under the same "lower bound, not exhaustive" honesty the PR now states, and I'd add one thing the PR doesn't: my resolved-import scan found 79 distinct registry classes reachable through a plain EnumField, not 73 — the difference is fully explained by four alias names each covering more than one real class (Enum_Option→3 distinct classes in tcp/mh/ipv6, Enum_Packet→3 in ipx/mh/ospf, Enum_Parameter→2 in hip/sctp, Enum_RouterAlert→2 in ipv4/ipv6: (3−1)+(3−1)+(2−1)+(2−1) = 6, and 73+6 = 79). I confirmed each colliding pair/triple was probed as the separate classes they are (e.g. tcp.option.Option is one of the 10 gaps, mh.option.Option and ipv6.option.Option are not), so this is a labeling nit in the "73", not a coverage gap in the "10" — non-blocking. No isinstance(x, <one of the 10 registries>) chain exists anywhere in pcapkit/ (checked by grep against both the aliased and bare class names), so there's no downstream branch that could silently take the wrong path against the new pseudo-member shape, for any of the 10.

3. Width guards on the seven flag registries — CONFIRMED not weakened. grep -rn "raise " pcapkit/const/ returns exactly one non-ValueError(...) line (a bare raise ValueError in reg/apptype.py) — every guard in pcapkit/const/**, including all seven named in the brief, raises a plain ValueError, never a pcapkit.utilities.exceptions type. And none of the seven (nor reg.apptype.TransportProtocol, the other width-bounded IntFlag) is ever passed as namespace= to an EnumField — confirmed by grepping the whole tree for namespace=<each of the seven/eight class names> (zero hits) and separately confirming BitField (what every one of the ~50 flags:/pcp:-with-a-dict schema fields actually uses) is a _TextField subclass, unrelated to NumberField/EnumField entirely. pcapkit.const.tcp.flags.Flags specifically: zero hits for namespace=Enum_Flags/namespace=Flags anywhere.

4. Exception selection — CONFIRMED, and the premise needs a small correction. The diff itself never chooses an exception class — it only re-raises (bare raise, same object, same traceback) whatever self._namespace(value) already threw, when that's a pcapkit.utilities.exceptions.BaseError subclass. BoolError (BaseError, TypeError — "must be bool") isn't used anywhere in pcapkit/const/ today (zero hits), isn't used by this diff, and isn't used by the new test's stand-in registries, which correctly use FieldValueError. So I could not find the specific "BoolError misuse" instance the brief warns is a recurring pattern in this diff or its tests — flagging as could not verify that this diff repeats it (I found no candidate site to check it against); the caution to watch for it was sound in principle but doesn't land on this PR.

5. breaking — CONFIRMED, reproduced exactly. Built a one-frame classic pcap with Ethernet EtherType 1501. main: frame.protochain == 'ETHERNET', type(frame.payload).__name__ == 'Raw'. Branch: frame.protochain == 'Ethernet:<unassigned>', type(frame.payload).__name__ == 'Ethernet', repr(frame.payload.info.type) == '<<unknown>.<unassigned>: 1501>' — matches the PR's before/after table verbatim. I didn't find anything else in the 85-line numbers.py diff that changes behavior beyond what's described: the except ValueError/isinstance(error, BaseError) re-raise preserves the exact exception object for the in-library case (bare raise inside except, not a new construction), and the fallback pseudo-member construction is the same three lines the no-namespace branch already ran, unconditionally moved rather than changed.

6. EXPECTED_FAILURES — CONFIRMED. tests/protocols/test_option_roundtrip_unit.py is byte-identical between 9d7890db4 and f0af61077 (diff empty), so its 44 entries are necessarily unchanged; imported and counted directly (44, listed, matches). Ran the six-test suite on the branch: 6 passed, including test_round_trip_is_identity_or_a_recorded_gap — green, so no recorded gap flipped to passing.

7. Tests fail without the fix, aren't vacuous, no hidden subtest failures — CONFIRMED, on both 9154383b0 and f0af61077. pytest -v (not just unittest, to get pytest's own subtest reporting) on main + either test-file version: 5 failed, 6 passed, ..., 2 subtests passed, exit 1, with the 5 failures being exactly the ValueError-from-aenum ones named in the PR. On the branch (either head): 11 passed, ..., 2 subtests passed, exit 0, no SUBFAILED anywhere. Built the "blanket except ValueError: pass" variant by hand (deleted the isinstance(error, BaseError): raise guard) and reran just test_an_in_library_rejection_is_not_absorbed: 2 failed, 1 passed, with SUBFAILED(namespace='StdlibBounded') and SUBFAILED(namespace='AenumBounded') — confirms the test distinguishes the real fix from the naive one, exactly as claimed, and is reported as failed (not masked as a false PASS).

8. Scope — CONFIRMED. git diff 9d7890db4...f0af61077 --stat touches exactly pcapkit/corekit/fields/numbers.py (+85/−8) and the new tests/corekit/test_fields_numbers_unassigned_enum.py (+436/0). No CHANGELOG.md, no pcapkit/const/** (git diff ... -- CHANGELOG.md pcapkit/const/ is empty).

Two things from the coordinator's follow-up brief, answered directly:

  • Is the in-library-BaseError passthrough dead code with a test for company? I'd call it a documented extension point, not dead code — it's the thing that makes this not a blanket except ValueError: pass (verified in Reconstruction accomplished #7 above), it's exercised by purpose-built stand-ins precisely because no first-party pcapkit.const registry needs it today (also verified: zero in-library raises anywhere in pcapkit/const/), and it protects any registry a caller supplies from outside pcapkit.const — which this is a public, extensible library, so that's a real and not merely theoretical audience. Sound.
  • The four post_process overrides — read all four directly (PortEnumField in tcp.py, udp.py, sctp.py; OptionEnumField in pcapng.py): all four call super(EnumField, self).post_process(value, packet), which the MRO sends straight past EnumField.post_process to NumberField.post_process. Confirmed by reading the source, not by instrumentation, but same conclusion: AppType and OptionType are out of scope by construction.

Could not verify: exhaustive (100%-of-space) coverage of the four remaining 32/128-bit registries beyond BlockType (ErrorCode, SecretsType, PayloadProtocolIdentifier, CGAType) — sampled only, per above. The specific BoolError-misuse precedent claim 4 warns about (no instance found in this diff to check it against). Full reconciliation of the PR's own "106 statically resolvable of 112" figure against my AST scan (I did not separately try to identify which 6 sites are the "not resolvable without evaluating the call" ones; mh.py's br_code_selector/fb_code_selector each return one of two literally-named EnumField(namespace=...) calls depending on a runtime condition, so I'd have counted their namespaces as statically known 4 sites rather than 6 unresolvable ones — this is a wording/methodology difference, not something I could find to be wrong, so I'm flagging it as unresolved rather than asserting either count).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Alters public API or wire output (apply alongside the type label) bug test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An unassigned PCAP-NG Block Type raises a bare ValueError from aenum, so UnknownBlock is unreachable and one unknown block costs the whole extraction

1 participant