fix(fields): fall through to the bounded pseudo-member instead of minting (#575)

[JarryShaw]

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• make test passes, and a test case covers the change
• Added a changelog entry — N/A, changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• fix — corrects a defect
• feat
• perf
• refactor
• test
• docs
• ci
• chore

---

Description

TCP/UDP/SCTP's PortEnumField and PCAP-NG's OptionEnumField called .get() on every parsed value, minting a member via aenum.extend_enum for anything no row or documented span covers — 111 calls on http.pcap, all ephemeral ports. All four now peek at what .get() would consult and call it only once the value is resolvable; a genuine miss gets EnumField._unregistered_member, a real member of the same registry built through its storage base's __new__, so isinstance holds and no lookup table grows (the owner's ruling; the broader principle is #775).

Riding along: the port fields check the value against the field's own byte width first, so #764's out-of-range rejection still raises instead of being absorbed; make_dumper escapes &/</> for PLIST-rooted output, which dictdumper never did (JarryShaw/DictDumper#125, tracked as #772); and PCAP-NG option namespaces stop cross-contaminating — on main, once a code was minted under opt, OptionType.get merged opt over the requested namespace and 6 of the 7 all answered opt_unknown for it, where each now gets its own <ns>_unknown.

Measured: extend_enum 111 → 0 on http.pcap and 15 → 2 on many_interfaces.pcapng (both from the untouched _missing_); extract(nofile=True) over 7 fresh processes per tree at host load 1.29 → 1.27, 0.952 → 0.679 s and 0.130 → 0.103 s respectively.

#575 asked for byte-identical output across the sample captures, as #420 and #427 did, and this deliberately misses that bar: 14 of 23 captures change in all four formats, entirely from the new <unassigned> rendering plus #772's PLIST escaping. Normalise those two and all 14 are byte-identical with no residual difference — that rendering change is the fix, hence breaking.

A pickle round-trip of a resolved unassigned member worked before this change (the member was minted, so registered) and would have broken after it, on read-back only since dumps succeeds either way. _unregistered_member now installs a __reduce_ex__ rebuilding an equivalent unregistered member; verified on protocols 0–5, both picklers, and copy/deepcopy on 3.11+ and with Enum.__copy__/__deepcopy__ deleted to emulate 3.10.

Unit tests cover the no-mint, isinstance, out-of-width, equality-without-identity, value-lookup-still-raises, in-library-rejection, round-trip, escaping, addon-key-order, int/TypeError-branch and pickle/copy paths — each shown to fail without its fix.

Fixes #575.

[JarryShaw]

Decision needed: where to fix the plist escaping this PR exposes. Also adding breaking — reasoning below.

CI: 4 failures, all one test. tests/integration/test_output_formats.py::PlistReportTests::test_plist_report_is_well_formed_xml_with_one_key_per_frame, on Integration 3.10/3.12/3.14 and parity 3.11, identically:

xml.etree.ElementTree.ParseError: mismatched tag: line 272, column 47

Root cause: the pseudo-member renders as <unknown>::<unassigned> [54321], and dictdumper's PLIST writer emits it unescaped — <string><unknown>::<unassigned> [54321]</string>. Reproduced in three lines against dictdumper alone, no pcapkit involved; filed as #772. It is live on main already for any genuinely unassigned option code, and this PR makes it reachable on essentially every capture because ephemeral ports are ubiquitous.

Four ways to resolve it, and this is not mine to pick:

1. Escape per-format in make_dumper's hook — only the plist/xml writers need it; < is legal in json/text/tree. Keeps the established spelling, touches pcapkit/dumpkit/ only.
2. Rename the pseudo-member to drop the angle brackets. Smallest diff, but changes the dump spelling EnumField.post_process's docstring names verbatim, and dumpkit's object_hook interpolates o.name unguarded, so a nameless flag member renders as Type::None [0] #648/An unassigned PCAP-NG Block Type raises a bare ValueError from aenum, so UnknownBlock is unreachable and one unknown block costs the whole extraction #701 may pin it.
3. Fix dictdumper upstream and pin a new release. Correct, but not in this repo and would block this PR indefinitely.
4. Escape inside render_enum — rejected on inspection: it would double-escape in the four formats where < is fine.

I would take (1): it fixes #772's symptom where it actually matters, leaves the spelling alone, and does not wait on an upstream release. But it widens this PR into pcapkit/dumpkit/, so say which you want.

On breaking, now applied. Measured either side of the change, for an unassigned port:

main :  <TCP.unknown: 54321 [tcp]>        type=TCP        registry 6147 -> 6148, name present
here :  <<unknown>.<unassigned>: 54321>   type=<unknown>  registry 6147 -> 6147, name absent

.port, .svc and .proto are preserved, so most callers are unaffected — but the returned object's type changes and the member no longer enters the registry, which is observable. #764 carried breaking for a narrower change, so this qualifies.

What is confirmed good: extend_enum on http.pcap goes 111 calls → 0, and on many_interfaces.pcapng 15 → 2 with both survivors coming from _missing_'s bounded declared-range path, which is correct and stays. That is #575 fully resolved.

[JarryShaw]

go with your recommendation (option 1) and file an issue to DictDumper.

[JarryShaw]

should we actually raise if it's an illegal value since the self._namespace enum rejected it? instead of creating a pseudo value.

[JarryShaw]

Good question, and it splits into two cases that this PR currently conflates — you are right about one of them.

For an in-range but unassigned value, the pseudo-member is correct and raising would be a regression. The namespace did not reject 54321 as illegal; it has no name for it. Port 54321 is perfectly legal wire data — IANA simply assigns nothing individually in 49152-65535. EnumField.post_process's own docstring (written for #701) spells out what raising cost: the bare ValueError is aenum's, not one of pcapkit.utilities.exceptions, so a caller cannot tell it from a bug of its own, and it is not an EOFError, so Extractor.record_frames does not catch it — one unassigned code cost the whole extraction. It also made the unknown readers the formats require unreachable: PCAP-NG's UnknownBlock and the unassigned option readers of IPv4, TCP, HOPOPT, MH and HIP, because the lookup failed frames before the dispatch that would have selected them. PCAP-NG repeats a block's length at both ends precisely so an unrecognised block can be skipped; that skip is what the fallback restores.

But for a genuinely illegal value it should raise, and right now it does not. Measured on this branch:

54321 (in-range, unnamed) -> <<unknown>.<unassigned>: 54321>     intended
80    (assigned)          -> <TCP.http: 80 [tcp|udp|sctp]>       correct
-1    (ILLEGAL)           -> <<unknown>.<unassigned>: -1>        *** ABSORBED ***
70000 (ILLEGAL)           -> <<unknown>.<unassigned>: 70000>     *** ABSORBED ***

#764 made AppType.get raise for anything outside 0-65535 — a breaking change, merged as 57b2c1761. This PR silently undoes it for these four fields. The type-based rule ("absorb any bare ValueError") is too coarse: it cannot tell aenum's "no member has this value" from #764's deliberate range rejection, because both are bare ValueErrors.

The PR's own reasoning was that the range guard is unreachable from parsing since the fields are PortEnumField(length=2, …), so 0-65535 is guaranteed. That is true of the parse path, but post_process is reachable directly, and more to the point it means the absorption buys nothing while quietly reverting a fix that landed deliberately.

So: keep the pseudo-member for unassigned-but-legal, and let an out-of-range rejection propagate. Routing that as a required change.

[JarryShaw]

I think for in-range unnamed values, we should still return the self._namespace enum tho, rather than creating a new one. so that means we should extend_enum on the self._namepace on that case, which I was expecting the enum itself to handle it.

[JarryShaw]

Measured this before answering, and there is a third option that gives you what you want without the extend_enum — which matters, because extend_enum is precisely #575's cost.

str.__new__ on the namespace class produces a genuine member of it that is never registered:

obj = str.__new__(TCP, 'unassigned')
obj._name_ = 'unassigned'; obj._value_ = 'unassigned'
obj.port = 54321; obj.svc = 'unknown'; obj.proto = TransportProtocol.tcp

repr             : <TCP.unknown: 54321 [tcp]>
isinstance TCP   : True        isinstance AppType : True
.port/.svc/.proto: 54321 / unknown / tcp
registry grew    : 6147 -> 6147 members, 6147 -> 6147 values

So it is a self._namespace enum — isinstance(x, TCP) holds, and it reprs identically to a declared member — while the registry does not grow. That satisfies your requirement and #575's at the same time, where extend_enum can only satisfy yours: minting is what costs 111 calls and ~13.5% of extraction self time on http.pcap, which is the whole of #575.

Note TCP is a StrEnum, not int-based — MRO is TCP → AppType → StrEnum → str → ReprEnum — so it is str.__new__, not int.__new__; the latter raises TypeError: TCP is not a subtype of int.

Three caveats I have not resolved, and they decide whether this is actually better than the <unknown> pseudo-enum now in the PR:

1. TCP(54321) still raises — the instance is absent from _value2member_map_, so it is not recoverable by value lookup. Same as the current pseudo-member, but worth being explicit that "returns a TCP" does not mean "findable as a TCP".
2. AppType defines __eq__/__hash__ on .port, so two unregistered instances for the same port would compare equal without being identical. Whether anything depends on is needs checking.
3. Pickling and copy may not survive an unregistered member. If any dump path pickles these, that breaks.

Also, on "I was expecting the enum itself to handle it" — that would put it in AppType._missing_, which is where aenum would naturally hook. I steered the worker away from _missing_ on your earlier ruling that get() is the public API and should keep minting for direct callers. If you would rather the enum own this, say so and I will move it there instead of into the field classes.

Which do you want: this unregistered namespace member, or the <unknown> pseudo-enum the PR currently returns? The out-of-range fix is unaffected either way and is already routed.

[JarryShaw]

this is actually quite a nice workaround and i think we should even apply to all other Enum's legit but unbounded values - so that we dont create registered enums out of unrecognised/unregistered values, unless user/caller explicitly created them.

[JarryShaw]

Ruling recorded, in your words: "this is actually quite a nice workaround and i think we should even apply to all other Enum's legit but unbounded values - so that we dont create registered enums out of unrecognised/unregistered values, unless user/caller explicitly created them."

Taking the unregistered namespace member for this PR. But measuring the general rule's reach turned up something that corrects a claim I made earlier in this thread: the declared-range _missing_ branches mint per value as well.

TCP.get(49100)   -> <TCP.unassigned: 49100 [undefined]>   members 6147 -> 6148
TCP.get(49101)   -> <TCP.unassigned: 49101 [undefined]>   members 6148 -> 6149
LinkType(60000)  -> <LinkType.Unassigned_60000: 60000>    members  220 ->  221

I had described those as "the bounded declared-range mechanism — the registries doing their job", against the unbounded get() fall-through. Wrong: they are bounded in which ranges they cover, not in how many members they create. A capture touching 200 distinct ports inside one reserved span grows the registry by 200.

So your principle reaches both paths, and the scope is:

extend_enum inside _missing_ : 1056 calls across 113 files
extend_enum inside get()     :  113 calls across 112 files

That is 1,169 sites across 113 registries — far beyond this PR. So I am splitting it: #771 keeps its four EnumField subclasses, which is where the measured 111 calls and ~13.5% of extraction self time live and is what closes #575, and the general rule gets its own issue with this measurement in it. Say if you would rather #771 carry the whole thing; I would not, because a 113-registry change with a breaking label and no incremental verification is not reviewable.

Two things your wording settles that were open:

• "unless user/caller explicitly created them" — so a direct AppType.get(54321) may still register, since that is an explicit caller action. That matches your earlier ruling that get() is the public API. The parse path never asked for a name, so it gets the unregistered member.
• It does not settle whether AppType._missing_ should own this rather than the field classes. For fix(fields): fall through to the bounded pseudo-member instead of minting (#575) #771 I am keeping it in the field classes, per your earlier ruling. The general issue is where the _missing_ question belongs, since that is the path with 1,056 sites.

The three caveats I raised still need answering there rather than here: TCP(54321) stays unfindable by value lookup, __eq__/__hash__ key on .port so two such instances compare equal without being identical, and pickling an unregistered member may not survive.

[JarryShaw]

for .get(), it's not an explicit call. I say we can probably add something like a .register(...) call that takes code, name, and any other required values to generate a proper enum as an explicit call.

for _missing_, it is still scoped to those ranged bounded but not expanded branches, like for ports 100-200, they are all reserverd_nnn branch.

this ruling must apply to all enums under pcapkit.const, not just the AppType classes.

[JarryShaw]

Recorded, in your words: "for .get(), it's not an explicit call. I say we can probably add something like a .register(...) call that takes code, name, and any other required values to generate a proper enum as an explicit call. for _missing_, it is still scoped to those ranged bounded but not expanded branches, like for ports 100-200, they are all reserverd_nnn branch. this ruling must apply to all enums under pcapkit.const, not just the AppType classes."

That corrects me — I said a direct AppType.get(54321) counted as an explicit caller action and could keep registering. It does not, and the consequence is that registration becomes opt-in through a new API rather than a side effect of lookup. Carried into #775 with the two measurements below.

#771 is unaffected and stays as it is. It only touches the four EnumField subclasses' post_process, never get() or _missing_, so its scope does not move. CI is green at 57c82794e and the switch to the unregistered namespace member is with the worker.

Two things measured that shape the .register() design, neither resolved:

1. .register() cannot have one signature. Member shapes genuinely differ — AppType.__new__(cls, value, name='<null>', proto=TransportProtocol.undefined) carries port/svc/proto, while LinkType(IntEnum) is a plain IntEnum with no custom __new__ at all. So "code, name, and any other required values" is per-registry. Either .register() lives on each class with its own signature, or it takes **kwargs and each registry validates — the second keeps one call site but moves the error from signature to runtime. Worth deciding before anyone writes it. Nothing named register exists under pcapkit/const/ today.

2. Three existing tests depend on minting through lookup and will need rewriting to .register():

tests/const/test_const_apptype_split_unit.py:161  addCleanup(_purge_member, TCP, 'PORT_59001_tcp', 59001)
tests/const/test_const_apptype_split_unit.py:178  addCleanup(_purge_member, UDP, 'unit_test_80', 80)
tests/const/test_const_apptype_split_unit.py:272  addCleanup(_purge_member, TCP, 'PORT_51_tcp', 51)

Your _missing_ scoping is exactly what I measured — those branches mint one member per distinct value inside a declared range, so a capture touching 200 ports in a reserved_nnn span grows the registry by 200. Full scope stands at 1,169 sites across 113 registries (1,056 in _missing_, 113 in get()).

[JarryShaw]

1. .register(...) lives per register with its own signature.
2. update the tests then.

[JarryShaw]

Cross-review at 42efb070b (different model, briefed to falsify): NEEDS CHANGES — three real defects, none of them in the mechanism.

1. A docstring sentence that this PR's own test contradicts 30 lines away. pcapkit/corekit/fields/numbers.py:641-644 says a value-keyed lookup — "self._namespace(value), or AppType.get called again with the same port — still raises exactly as it did before this existed." get() does not raise; it mints. Measured on this head: post_process(53406) leaves the registry at 6147, then TCP.get(53406) grows it to 6148 as PORT_53406_tcp. Only the self._namespace(value) half is true, which is exactly what test_a_value_lookup_on_the_resolved_port_still_raises says in its own docstring. Strike the get() clause.

2. Dumped key order flips for unassigned members only. The three port sites pass port=value, svc='unknown', while AppType.__new__ sets obj.svc then obj.port (apptype.py:2289-2291). _unregistered_member populates __dict__ in kwargs order, so a declared member renders svc, port and an unassigned one port, svc — ~2638 lines across the JSON dumps. One-line fix at three sites; the pcapng.py:603 site wants the same check against OptionType.__new__.

3. Two new branches are unexecuted by any test. numbers.py:708-711, the int.__new__ branch and the TypeError guard. Both namespaces in play are str-based, so the int branch is dead for all four production call sites. Correct when exercised directly — a coverage gap, not a defect, but this repo's rule is that every change raises coverage.

Correcting the review on its fourth point. It reported that the body does not disclose the copy/pickle limitation. It does, at body line 48. But the wording mis-describes it: "raise on 3.10 exactly as pickle does everywhere" implies pickle already failed. Measured on CPython 3.14.7, one unassigned port through the real field:

	83b58ebda	42efb070b
copy / deepcopy	OK	OK
pickle.dumps	OK (80 B)	OK (80 B)
pickle.loads(dumps(m))	OK	ValueError: 'unknown [53406 - tcp]' is not a valid TCP

So a pickle round-trip worked before this change and does not now, on 3.14 as well as 3.10, and dumps still succeeds — a persisted dump fails only on read-back. That is a regression on a breaking-labelled change described as a pre-existing limitation. Restate it as one, or override __reduce_ex__ on the unregistered member.

Two prose gaps. An unclaimed improvement: PCAP-NG option namespace cross-contamination is gone — on main, once code N was minted in opt, every other namespace's lookup for N returned opt_unknown. And #575 asked for byte-identical output plus wall-clock timings; 14 of 23 captures change and the body addresses neither.

Verified green, independently derived: extend_enum 111 → 0 on http.pcap (15.2/13.9/13.6% of tottime at base), every surviving call a _missing_ frame with no get() caller; a 655,360-row sweep across all ten registries with 0 regressions and exactly one transition, would-have-minted → unregistered, same owner/value/svc/proto; #764's per-transport divergence intact; out-of-width still raises, including under signed=True; EXPECTED_FAILURES imported (43 entries, 0 touching unassigned); CI 26 success / 4 deliberately skipped on this head. The Python 3.10 copy failure that killed 731d8b27a is fixed and is independent confirmation of the 3.11 Enum.__copy__ boundary.

Routed to a worker. Label moves to review: needs-changes.

[JarryShaw]

Round-two cross-review at f42e93d31 (sonnet, a different model from the author): GOOD TO GO, no required changes.

The mechanism attack found nothing. Reproduced independently: _rebuild_unregistered_member and _reduce_unregistered_member route only through _unregistered_member, never namespace.__new__, so the rebuild path cannot mint — restoring a stale pickled member after a direct AppType.get(54321) had grown the registry to 6148 leaves it at 6148, adding nothing. _unregistered_member's body never touches __registry__, so the None-registry branch is unreachable from here. Both the C _pickle accelerator and pure-Python pickle._Pickler honour the instance-level __reduce_ex__ for protocols 0-5, and removing the override reproduces the exact failure it exists to prevent: ValueError: 'unknown [53406 - tcp]' is not a valid TCP on read-back. It works because pickle and copy getattr __reduce_ex__ off the instance rather than through implicit slot lookup — probed directly, not assumed. No reference cycle: the partial's bound args never reference obj, and a weakref confirms the member is collectable immediately after del. Nothing leaks into dumped output across real json/tree/plist/xml of in.pcap, with 5 unassigned entries present in each. I had separately measured all six protocols round-tripping with the registry flat at 6147 and copy/deepcopy working under the 3.10 emulation, so this is two independent confirmations.

The 14-captures arbitration: the worker is right and round one undercounted. Fresh 23-capture sets on both trees, all four formats, 92 files each: exactly 14 captures / 56 files differ, and normalising #772's entity escaping plus the minted-name rename leaves 0 residual differences across all 14. The reason round one said 13 is concrete — there are two mint-name shapes, and its regex covered only the first: PORT_%d_%s at pcapkit/const/reg/apptype/apptype.py:2423 for AppType, and %s_unknown_%d / opt_unknown_%d at pcapkit/const/pcapng/option_type.py:216,233 for OptionType. I checked both call sites myself. And test.pcapng is not unparseable: it extracts cleanly on both trees with 5 × "error": null. It fails Python's strict json.load only because of a pre-existing dictdumper quoting bug, identical on both trees, when a raw-bytes dict key contains ", # or % — a real but orthogonal defect, not the thing the byte-diff claim needed.

Verified green. Coverage under coverage run --branch: the whole _unregistered_member body (:605-754) and both new module functions (:757-807) show zero missing lines or branches; the only numbers.py misses are pre-existing and outside this round. Each new test shown to fail without its fix — reverting the svc/port kwarg order in tcp.py fails test_an_unassigned_port_dumps_its_addon_keys_in_the_declared_order exactly. 28 passed / 38 subtests under pytest, Ran 28 tests ... OK under unittest, in agreement. mypy and pylint identical at 42efb070b and this head — one pre-existing [misc] and one f-string-without-interpolation, both files 9.93/10. Timings reproduced at ~0.94 → 0.69 s median on http.pcap. CI all success or expected skips, Python 3.10 included — the leg that killed 731d8b27a on the copy path.

Three non-blocking notes. The unused-argument pragma works as intended, but naming the parameter _protocol would have needed no pragma at all, since the repo leaves pylint's default ignored-argument-names = _.*|^ignored_|^unused_ unmodified; the repo's own multi-line precedent at pcapng.py:150 also puts the pragma on the unused arg's line rather than the bare def. The body's changelog note reads N/A, changelog centralised in #657 with a comma rather than an em-dash — cosmetic. And the closing "Unit tests cover…" paragraph could go to reach the ~20-line target without losing a measured fact.

One real gap, tracked rather than blocking: #780. docs/source/pcapkit/corekit/fields/numbers.rst needs .. autofunction:: stubs for the two new module-level helpers under its existing "Internal Definitions" heading. The file was legitimately outside the worker's owned set, and the repo's precedent (pcapkit/foundation/engines/_pcap_backend.py's _purge) documents exactly this shape with individual stubs rather than a blanket :private-members:, which is commented out repo-wide. It cannot land before this PR merges, since the functions do not exist on main yet.

Three PRs are now ready to merge: #771, #774 and #777.