fix(corekit): make EnumRegistry.get's no-default marker an identity sentinel (#857)

[JarryShaw]

Please follow the guide below

• 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 — not run as such (it OOMs on this box); ran the targeted suites named below instead, and a test case covers the change
• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — 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

---

Description of your pull request and other information

Closes #857. EnumRegistry.get's no default marker was the magic value -1 compared with ==, so a caller's own -1 — or worse, -1.0, since -1.0 == -1 — collided with the marker silently. NO_DEFAULT is now compared with is at both sites.

Owner ruling on #859 ("use dedicated class rather than bare object. Follow the house convention") replaced the first revision's bare object() with a dedicated NoDefaultType singleton, matching NullType (pcapkit/corekit/module.py) and NoValueType (pcapkit/corekit/fields/field.py):

• Class name follows the settled half of the convention, <Name>Type. The instance keeps the existing name NO_DEFAULT rather than being renamed to either precedent's casing — the owner's follow-up says instance naming is unsettled and "depends on how we need it," and the need here is continuity: NO_DEFAULT is already the name on main, so this change is to what the sentinel is, not what it's called.
• __new__ returns a cached singleton — guards against a caller writing NoDefaultType() themselves and getting a second, non-identical sentinel that would silently fail is NO_DEFAULT.
• No __bool__ — deliberately, unlike both precedents. NO_DEFAULT is checked only by is, never in boolean context, and a falsy sentinel would invite exactly the truthy/falsy conflation (if not default:) this fix removes.
• No __copy__/__deepcopy__/__reduce__ — object.__reduce_ex__ at protocol ≥ 2 already routes through the guarded __new__ (copyreg.__newobj__ → cls.__new__(cls)), so copy.copy/copy.deepcopy/pickle ≥ 2 already preserve identity with no extra code. Only pickle protocol 0/1 bypasses it (copyreg._reconstructor → object.__new__ directly), and that path never runs because the sentinel is never stored in anything this package pickles.
• Both NO_DEFAULT and NoDefaultType are now exported in __all__ (previously ['EnumRegistry'] only).
• Known, stated caveat: unlike -1 == -1 (immune, value comparison), reloading this module leaves get's already-bound parameter default holding the pre-reload singleton while the module global holds the fresh one, so an omitted default stops raising the original lookup error after a reload — worse than the marker it replaces. Nothing in this package reloads it; documented rather than hidden.

Full reasoning for each of these is in the NoDefaultType class docstring, corrected across two rounds of cross-review (the copy/pickle and reload reasoning above reflects the round-2 correction).

Scope: one file, pcapkit/corekit/enum.py — the sole consumer of NO_DEFAULT. Since #858 landed, 111 of 121 const registries inherit get() from this base, so this one-file change reaches all of them. No live correctness bug: no pcapkit.const registry's domain reaches -1 today, so this is a clarity fix.

Of the 10 bespoke registries that don't inherit this base, three (ftp/return_code, http/status_code, pcapng/option_type) carry their own hand-written -1-as-default convention in a local get() — left untouched, harmonising them is out of this issue's scope.

Also corrected tests/const/test_const_enum_get.py's test_the_placeholder_still_raises, which asserted "explicitly passing [-1] is the same as omitting it" — no longer true, so it now pins the corrected, distinguishable behaviour.

Ran the targeted suites this repo's tooling notes call for instead of the full make test (which OOMs on this box): tests/const (168 tests), tests/vendor (86) and tests/test_tier_guard.py (107), all green via plain unittest. mypy/pylint/isort clean on the changed source file (one pre-existing, unchanged mypy finding on enum.py's Self.name access).

[JarryShaw]

NEEDS CHANGES on 81b367d43 — cross-review (opus, a different model from the sonnet author). The code change is
correct and minimal. breaking is now applied and the PR body is corrected; one item is left and it is not in
this PR.

1. breaking applied. The measured case: Cls.get('<missing name>', -1) goes KeyError → ValueError, and
ValueError is not a subclass of KeyError. A caller who wrote try: Cls.get(name, -1) except KeyError: — following
both the documented -1 placeholder and get's own documented Raises: KeyError — now has an uncaught exception
escape. Same class as #855/#858, on the same base class, reaching the same 111 registries. 1.5.0a1 through b4
shipped as published prereleases, so installed code can genuinely be passing -1.

2. Still outstanding — docs/source/changelog/1.5.0.rst:802-808 now states the opposite of shipped behaviour.
The #584 entry reads, verbatim:

-1, the placeholder the generated signature already carried, is what separates "no default was supplied" from
"a default was supplied and should be used", so a caller that asked for no fallback still gets the error rather
than a silent substitution.

After this PR that is false for the 111. Both statements would ship in the same unreleased 1.5.0. The changelog
box here is correctly ticked N/A — centralised in #657, but that covers adding an entry, not an existing entry
this PR falsifies. #657 owns that file and its worker is fixing it — amending there is safe because #657 merges
last, after this.

3. PR body corrected. It lumped pcapng/option_type in with the two files carrying a -1-as-no-default
convention. Verified: option_type has no default == -1 check — the value goes straight to
extend_enum(OptionType, key, default, key) at option_type.py:219 as the value to mint. Only ftp/return_code
and http/status_code carry the real convention, and each documents it inline.

My own framing was wrong and the review caught it. I wrote that Hardware.get(99999, -1) "used to raise
KeyError/about 99999". On the value path it raised ValueError about 99999 — KeyError is the name
path only (cls._member_map_[key]). That distinction is the breaking case, so getting it loose mattered.

The strongest thing measured in the PR's favour, and it is worth recording: across 20 cells on an IntEnum and
an IntFlag, nothing went from success to failure, and nothing from failure to a wrong success. Every -1/-1.0
cell raised before and raises after. The reviewer then established why mechanically rather than by assertion — it
walked all 127 Enum classes in pcapkit.const and found 0 members with a negative int value, so -1 cannot
be a legitimate lookup anywhere. Zero registry mutation in all 20 cells, both trees.

Also confirmed: the six new tests fail on base with real output — AssertionError: -1 == -1, -1 == -1.0, two
KeyError escapes, and AssertionError: '-1' not found in '99999 is not a valid Hardware'. The rewritten
test_const_enum_get.py test is a strengthening: it adds assertIn('-1', …) and assertNotIn('99999', …),
pinning which key the error names instead of conflating the two calls. NO_DEFAULT is module-level and identity-stable
across imports; nothing in docs/ or pcapkit/vendor/ references it; the pre-existing mypy "Self" has no attribute "name" finding is unchanged, only shifted two lines.

One census correction to my own numbers: I have been saying "121 modules / 10 bespoke". Mechanically it is 127
Enum classes, 111 inheriting EnumRegistry, 16 not — my 121/10 counted modules, the reviewer's counted
classes, and the extra six are auxiliary enums like ftp.command.CommandType and
ftp.return_code.GroupingInformation. The load-bearing 111 is right either way.

Not blocking, recorded for whoever touches this next: inspect.signature(Hardware.get) now renders
default: 'Any' = <object object at 0x…>, an address that changes per process. Not live today — there is no
docs/source/pcapkit/corekit/enum.rst and conf.py:110-119 has members commented out with no inherited-members,
so no Sphinx page renders the inherited get. But the comment at enum.py:61-65 justifying object() over a named
sentinel claims a sentinel subclass "can still collide … via a custom __eq__", which is not true of a subclass
defining only __repr__ — it inherits identity __eq__ and is exactly as safe. Either use a three-line
class _NoDefault for readable signatures, or soften that comment, because as written it overstates.

[JarryShaw]

GOOD TO GO on 81b367d43, 59✅/0❌/0. Nothing in this PR's diff needed changing — of the review's three required
items, two were mine and are done, and the third is not in this PR at all.

1. apply `breaking`                          -> done by me, label applied
2. PR body wrongly grouped pcapng/option_type -> done by me, body corrected
3. changelog 1.5.0.rst:802-808 now false      -> NOT in this diff; tracked on #657

Item 3 is the one to keep an eye on. The #584 entry promises "-1 … is what separates 'no default was supplied'
from 'a default was supplied and should be used'", which this PR makes false for the 111 registries. That file
belongs to #657, whose worker is amending it now, and #657 merges last — so the contradiction cannot ship ahead of
its fix. It is worth stating rather than assuming, because the changelog box here is correctly ticked
N/A — centralised in #657, and that convention covers adding an entry, not an existing entry a PR falsifies.

The code itself is three lines — NO_DEFAULT = object() at enum.py:66 and both comparison sites == → is at
:142/:148 — and the review's own measurement is the best argument for it: across 20 cells on an IntEnum and an
IntFlag, nothing went from success to failure and nothing from failure to a wrong success, with zero registry
mutation on either tree. Every -1/-1.0 cell raised before and raises after; the change is an exception type on
the name path, which is precisely why it carries breaking.

Ready for you to merge. One non-blocking note recorded on the review comment above, for whoever touches this next:
inspect.signature now renders default: 'Any' = <object object at 0x…>. Not live today — no
docs/source/pcapkit/corekit/enum.rst exists and conf.py has members commented out — but the comment at
enum.py:61-65 justifying object() over a named sentinel overstates its case, since a subclass defining only
__repr__ is equally safe under is.

[JarryShaw]

Use dedicated class rather than bare object. Follow the house convention.

[JarryShaw]

Recorded, verbatim: "Use dedicated class rather than bare object. Follow the house convention."

Doing it. And the house convention is not a matter of taste here — there are two existing precedents in
corekit itself, which I read rather than inferred:

pcapkit/corekit/module.py          class NullType    -> NULL = NullType()      both in __all__
  :103 __new__ enforcing a single instance      :109 __bool__ -> False
  :113 __repr__ -> '<NULL>'                     :117 __copy__ / __deepcopy__ -> self
pcapkit/corekit/fields/field.py:27 class NoValueType -> NoValue = NoValueType()
  :30 __bool__ -> False                         #: NoValueType: docstring comment above the instance

So the shape is <Name>Type class plus a module-level singleton instance, exported, with a real __repr__.
NullType even documents the hazard that a module reload re-executes NULL = NullType() and produces a second
singleton — worth carrying across, since NO_DEFAULT is compared with is.

Two consequences worth stating, because they are why this ruling improves the PR rather than just renaming things:

• The signature becomes readable. Today inspect.signature(Hardware.get) renders
  default: 'Any' = <object object at 0x…> — an address that changes every process. With a __repr__ it renders the
  name. Not currently visible in the docs (no docs/source/pcapkit/corekit/enum.rst, and conf.py has members
  commented out), but it is what help() and any traceback show.
• The comment justifying object() was wrong and goes with it. enum.py:61-65 claims a sentinel subclass "can
  still collide … via a custom __eq__". Not true of a class that defines only __repr__/__bool__ — it inherits
  identity __eq__ and is exactly as safe under is. The cross-review flagged this as an overstatement and your
  ruling settles it.

Also: __all__ at enum.py:52 is currently ['EnumRegistry'] only, so NO_DEFAULT is unexported today. Following
module.py, the new type and its instance both get exported.

Label back to review: needs-changes and the worker is on it. Everything else on this PR stands — 59✅/0❌/0, and the
breaking label is unaffected, since the sentinel's identity semantics do not change, only its type and repr.

[JarryShaw]

There's also NoValue or other sentinels in the house. The naming convention is not settled. Mainly depends on how we need it.

[JarryShaw]

Recorded, verbatim: "There's also NoValue or other sentinels in the house. The naming convention is not settled.
Mainly depends on how we need it."

Taken, and you are right that it is unsettled — the two existing sentinels disagree with each other on the instance
name, which I had been treating as one convention when it is two:

pcapkit/corekit/module.py:154        NULL    = NullType()        SCREAMING_CASE instance
pcapkit/corekit/fields/field.py:36   NoValue = NoValueType()     CapWords instance

They agree only on the <Name>Type for the class. So the class name is settled, the instance name is not.

What the need dictates here, which is the part that decides it: NO_DEFAULT is already the name on main —
pcapkit/corekit/enum.py:64 reads NO_DEFAULT = -1 today. So this PR is changing what the sentinel is, not what it
is called. Keeping NO_DEFAULT means the diff stays "the value became a typed singleton", with no gratuitous rename
of a name already referenced in the signature, the docstring and two comparison sites. NULL is the precedent that
matches it stylistically.

So: instance stays NO_DEFAULT, new class named for it, and the worker picks the exact class name on that basis
rather than by copying either precedent mechanically. I have told it the convention is explicitly unsettled per your
ruling and that the deciding factor is the existing name, not symmetry with NullType/NoValueType.

Two related choices I have deliberately left to it with the trade-off stated, since "how we need it" cuts both ways:

• __bool__. Both NullType and NoValueType return False. NO_DEFAULT is a marker that no default was
  supplied, not a false-y value, and nothing in get() evaluates it in boolean context — both sites use is. Adding
  __bool__ -> False for symmetry would encode a claim nobody needs; omitting it diverges from both precedents.
• __new__ singleton guard. NullType has one, and documents the hazard it exists for: a module reload
  re-executes NULL = NullType() and produces a second singleton, so code holding the pre-reload instance fails an
  is check. That reasoning applies verbatim to NO_DEFAULT, which is now compared with is at two sites —
  NoValueType has no such guard, but it is also not identity-compared.

It will report both decisions with reasons. Say the word if you would rather settle either yourself.

[JarryShaw]

I suppose we should rename NULLType to NullType. And keep the SENTINEL object's type class naming as SentinelType for a house convention.

Let's document this house convention somewhere in the docs so that future maintainers and agent workers will understand the rationale and follow.

[JarryShaw]

Recorded, verbatim: "I suppose we should rename NULLType to NullType. And keep the SENTINEL object's type class
naming as SentinelType for a house convention. Let's document this house convention somewhere in the docs so that
future maintainers and agent workers will understand the rationale and follow."

Two of the three are already true, and I would rather show you that than do a no-op rename.

1. NULLType does not exist — it is already NullType. Measured on origin/main:

pcapkit/corekit/module.py:20   __all__ = ['NULL', 'NullType', 'ModuleDescriptor']
pcapkit/corekit/module.py:31   class NullType:
git grep -c NULLType            -> no matches anywhere in the tree

So nothing to rename. Say the word if you meant a different spelling somewhere and I have looked in the wrong place.

2. <SENTINEL>Type is already what #859 does. On head f0edbbcc6:

pcapkit/corekit/enum.py:54    __all__ = ['NO_DEFAULT', 'NoDefaultType', 'EnumRegistry']
pcapkit/corekit/enum.py:58    class NoDefaultType:
pcapkit/corekit/enum.py:159   NO_DEFAULT = NoDefaultType()

So all three sentinels now agree on the rule as you have stated it — the type class is the instance name in CamelCase
with Type appended: NULL/NullType, NoValue/NoValueType, NO_DEFAULT/NoDefaultType. Note that reading makes
the instance casing deliberately free, which matches your earlier "not settled … depends on how we need it": the
convention constrains the type name, not the instance.

3. Documenting it — doing that now, and folding it into the same page as the mint/unmint criterion you asked for on
#775. One page, docs/source/conventions.rst, titled Registry Conventions, because both are the same kind of thing:
a ruling a future maintainer or agent worker cannot derive from the code. It will carry the rationale, not just the
rule — a dedicated type exists so repr() is readable in a signature, help() and a traceback, which a bare
object() cannot give, and identity comparison is what makes a sentinel safe against any caller value.

Docs-only, so it goes straight to main rather than through a PR, per the standing rule. I will report the commit.

One thing I will flag in the page rather than silently resolve: NullType has __copy__/__deepcopy__/__reduce__
and NoDefaultType deliberately does not, because NULL is stored in a ModuleDescriptor field a caller can
deepcopy or pickle into, while NO_DEFAULT is never stored in any structure a caller copies. That is a real
difference in need, not an inconsistency, and worth writing down so the next person does not "fix" it.

[JarryShaw]

NEEDS CHANGES on f0edbbcc6 — round-2 cross-review (opus). The code is correct; the prose is not. Three
documentation fixes, no behaviour change asked for. All three re-measured by me.

R1. enum.py:96-103's reason for omitting __copy__/__deepcopy__/__reduce__ is factually false. It says
those hooks exist on NullType because deepcopy/pickle "can walk into and reconstruct, bypassing __new__
entirely." They do not bypass it:

NO_DEFAULT.__reduce_ex__(2) -> __newobj__ (NoDefaultType,)   # routes through cls.__new__
NO_DEFAULT.__reduce_ex__(0) -> _reconstructor                # object.__new__, the only bypass
copy is ND True | deepcopy is ND True | pickle p2 is ND True | pickle p0 is ND False

So the decision is right and the justification is wrong — the __new__ guard already covers copy, deepcopy and
pickle ≥2. The only gap is protocols 0/1, unreachable because the sentinel is never stored in anything a caller
pickles. Note the same false claim is pre-existing in NullType's own docstring at
pcapkit/corekit/module.py:56-59; out of scope to fix there, but this file restated it as its own reasoning.

R2. tests/const/test_const_enum_get.py:194 carries stale round-1 prose — ":data:NO_DEFAULT is now a private
:class:object sentinel". Both halves are false now: it is in __all__, and it is a NoDefaultType. The sister
docstring in test_const_registry_protocol.py was updated, so this is an oversight.

R3. The reload caveat understates what actually breaks, and this is the valuable finding. Measured on the head:

BEFORE reload  Hardware.get('missing') -> KeyError: 'Definitely-Not-A-Member'
AFTER  reload  Hardware.get('missing') -> ValueError: <NO_DEFAULT> is not a valid Hardware

The bound parameter default holds the pre-reload sentinel while the module global holds the post-reload one, so
omitting default stops raising the lookup error — it tries cls(<NO_DEFAULT>) instead. On base this was immune,
because -1 is -1. The docstring frames the reload caveat only as "a fresh class object with its own reset
_instance" and never says the omitted-default contract itself breaks, which is strictly worse than what -1
did. Say what breaks.

All three design decisions ACCEPTED, with the semantics confirmed unchanged from round 1: 14 of 20 cells
byte-identical across an IntEnum and an IntFlag, the 6 differing being the same 3 per registry, nothing
success→failure or failure→wrong-success, zero registry pollution. NoDefaultType() is NO_DEFAULT → True; a reload
does produce a second instance, so that part of the docstring is honest. bool(NO_DEFAULT) is True and no caller
in the repo would be misled either way — one if not default:-shaped grep hit, an unrelated --engine help string.

Two corrections to things I said. Round 1's "3 changed in exception class only" was imprecise: only 2 change
the class; the third keeps ValueError and changes the message (99999 is not a valid → -1 is not a valid),
because on base the caller's -1 was swallowed as the marker. And NoValueType is a much thinner precedent than I
claimed — measured: NoValueType() is NoValue is False (not a singleton) and repr(NoValue) is the default
<…object at 0x…>. So only NullType actually sets the pattern being followed. I have corrected that in the
conventions doc I am writing.

Two more, not blocking: the PR body ticks [x] make test passes while the same body says make test OOMs and was not
run — targeted suites were. And test_identity_survives_an_ordinary_second_import passes on the base tree too
(-1 is -1), so it guards nothing this PR introduces; harmless.

[JarryShaw]

GOOD TO GO on 899bbdb3b. All three documentation fixes are in and the diff since f0edbbcc6 is prose only,
so round 2's confirmation that the code is correct carries forward unchanged.

Verified by me:

diff f0edbbcc6..899bbdb3b   77/17 in enum.py, 2/1 in test_const_enum_get.py
  every changed line is docstring or comment -- no executable line moved
R1  grep 'bypassing __new__ entirely' -> 0        and :100/:107/:112 now name
    copyreg.__newobj__ and copyreg._reconstructor explicitly
R3  :147 "omitted no longer compares the pre-reload default against NO_DEFAULT"
    :157 quotes the measured  ValueError: <NO_DEFAULT> is not a valid Hardware
R2  grep 'private :class:`object` sentinel' -> 0  and :195 reads "instance of the
    dedicated NoDefaultType"

R1 now states the real mechanism rather than the false one: __reduce_ex__ at protocol ≥2 reduces through
copyreg.__newobj__ → cls.__new__(cls), so the __new__ guard already covers copy.copy, copy.deepcopy and
pickle ≥2; only protocols 0/1 go via copyreg._reconstructor → object.__new__, and that is unreachable because the
sentinel is never pickled. It also replaces the loose "no equivalent exposure" line by naming the two real exposure
points — get.__defaults__[0] and inspect.signature(...).parameters['default'].default — and saying why neither
opens a hole. NullType's own copy of the false claim at module.py:56-59 was correctly left alone as out of scope.

R3 is the one worth reading, and the author went further than asked: it verified on a fresh 4ec9e9558 worktree
that -1 is genuinely reload-immune (KeyError both before and after, since -1 is -1 holds regardless of which
module execution produced it), which is what makes the sentinel strictly worse than -1 under reload rather than
merely different. It cites the tracked defect class too — ProtocolBase._lookup_next_layer's docstring (#425/#428/
#560) and test_no_stale_class_survives_a_module_reload — and frames "nothing reloads this module" as a caveat to keep
honest rather than a guarantee. That is the right posture for a caveat that is currently theoretical.

Also done, both optional: the PR body's make test checkbox is unticked and qualified, naming the targeted suites
actually run. And test_identity_survives_an_ordinary_second_import was kept rather than dropped — it passes on base
too, but it now serves as the contrasting half of R3's story: an ordinary re-import is fine, only a reload is not. Fair
call.

tests/const 168, tests/vendor 86, tests/test_tier_guard.py 107, all OK under plain unittest; pylint 10.00/10;
the pre-existing mypy "Self" has no attribute "name" finding unchanged. Two cross-review rounds on opus against a
sonnet author.

Ready for you to merge. Its merge unblocks #860, which in turn gates 781 of #775's 1007 minting branches.