fix(corekit): fall EnumRegistry.get() back to a str key's value (#860)

[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
• 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

---

Description of your pull request and other information

Step 1 of #860 only: fixes EnumRegistry.get's str-key dispatch so a
StrEnum registry can resolve a key that is a valid value but not a
name. Step 2 (converting the 16 bespoke registries) is a separate,
still-open change.

The defect. get's str branch tried only cls._member_map_[key]
(a name lookup) and, on a miss, either raised or returned cls(default)
— it never tried the value path at all. On a synthetic EnumRegistry +
StrEnum fixture, _Str.get('known-value') raised KeyError even
though _Str('known-value') resolved fine. A supplied default
compounded it: it overrode a good value match the code never tried.

The fix. After the name lookup misses, fall back to a plain
_value2member_map_ dict lookup before consulting default. Deliberately
not cls(key): FEATCode._missing_ (pcapkit/const/ftp/command.py:43)
mints directly via extend_enum for any unrecognised string — confirmed
by reproducing its shape in a test — so routing the fallback through the
constructor would let a failed name lookup mint a permanent member the
moment step 2 converts a registry like it onto this base. A dict lookup
never reaches _missing_, so it cannot mint, on any registry.

Name still wins when a string is both a name and a different member's
value — matches today's behaviour for names that already resolve, pinned
with its own test.

Not touched: the IntEnum/IntFlag non-str path — its own get()
matrix (valid value/name, missing name/value with and without default, a
key that is neither int nor str) is measured identical before and
after, on both an IntEnum (TransType) and an IntFlag
(HandoverACKFlag). Nothing under pcapkit/const/ or pcapkit/vendor/.

Breaking? No — this widens: a call that raised KeyError before now
resolves. No caller that got a correct result before gets a different one
now.

Build/test: tests/const, tests/vendor, tests/test_tier_guard.py all
green (372 passed, re-derived under plain unittest too). mypy's one
pre-existing "Self" has no attribute "name" finding on this file is
unchanged.

[JarryShaw]

NEEDS CHANGES on 1e90155fc — cross-review (opus; author sonnet). One docstring clause, no code change. I verified the finding myself before relaying it.

get's docstring promises, unconditionally, that "A value inside a registry's declared-but-unassigned range still resolves, through that registry's own _missing_ and _unregistered_member, to a member that is deliberately absent from the lookup tables" (pcapkit/corekit/enum.py:270-273). That is now false for a str key. The new branch matches on _value2member_map_, and _unregistered_member (:501-503) builds its member through cls._member_type_.__new__ and hand-sets _name_/_value_ — it never enters either lookup table. The non-str path at :299 is cls(key), which does reach _missing_. So the two halves diverge:

Rng("reserved-9")      -> <Rng.Reserved_reserved-9: 'reserved-9'>
Rng.get("reserved-9")  !! KeyError 'reserved-9'
UnregInt.get(150)      -> <UnregInt.Reserved_150: 150>     # int key reaches _missing_

Latent today and not introduced here — on main the str value path did not exist at all. Worth fixing now because #861 is converting 172 branches to produce exactly those members, and in a 16-line diff of which 14 are docstring, the docstring is the deliverable. Add a clause saying the str value side matches only already-registered values.

Everything else confirmed, including what the review tried hardest to break: both halves of the defect reproduced independently on both trees; extend_enum measured returning a member, so a naive cls(key) would have made get('never-seen') succeed and register permanently; _value2member_map_ holds the canonical member for an alias and register_alias touches only _member_map_, so the name branch still covers aliases; IntEnum/IntFlag matrices byte-identical, including the get("6") / get("6", 17) probes the PR did not list; 372 passed / 40551 subtests, and 69 under plain unittest.

Two corrections to my own earlier statements. I said five StrEnum classes exist under pcapkit/const/; it is nine — I missed reg/apptype/{dccp,sctp,tcp,udp}.py, whose class DCCP(AppType): and siblings inherit the StrEnum through AppType and never spell it, so a grep for StrEnum cannot see them. The conclusion is unchanged: 111 EnumRegistry subclasses, value-key-type histogram {('int',): 111}, zero str keys, and none of the nine inherits EnumRegistry. So no breaking label — confirmed behaviourally too, since get("6", 17) returns the identical member on both trees. And the PR's "two pre-existing mypy findings" is one: mypy.ini:2 sets ignore_missing_imports = True, so the aenum untyped-import note appears only when mypy runs without the config. "Self" has no attribute "name" at :395 is the only finding, and 309 total on both trees.

[JarryShaw]

Round 2 pushed — new head 6cbe5eadd, still one commit, amended in place. enum.py is now +28/−0, all docstring; no code change and no rebase needed (origin/main had not moved).

The added paragraph carries the caveat the review asked for, and puts it on the PR's own new prose rather than editing the pre-existing sentence: a declared-but-unassigned value is invisible to the _value2member_map_ check precisely because _unregistered_member grows neither lookup table, so it resolves through cls(value) but not get(value) for a str key, while the non-str path always reaches _missing_. It also says the asymmetry is deliberate — closing it would mean calling cls(key) for a str value, reopening the minting hazard. Asymmetry reproduced independently before the prose was written.

Correction to my previous comment. I wrote that "the PR's 'two pre-existing mypy findings' is one". The PR body never said two — line 52 reads "mypy's one pre-existing ... finding", singular. The "two" was in the agent's hand-back report to me, not in this PR, and I misattributed it. The substance stands: one finding, "Self" has no attribute "name" at enum.py:395, 309 errors in 35 files on both trees, and the aenum note appears only when mypy runs without mypy.ini. No PR body edit was needed.

Also taken: the test_get_never_mints_on_a_registry_whose_missing_mints_directly docstring now scopes "verbatim" to the minting call (extend_enum(cls, value.upper(), value), pcapkit/const/ftp/command.py:52) and notes the fixture's non-str branch differs from the real one, calling that immaterial to what the test proves.

69 tests still pass under plain unittest; this round touched prose only. Label back to review: pending — the earlier NEEDS CHANGES was pinned to 1e90155fc. Re-review dispatched on the same head.

[JarryShaw]

NEEDS CHANGES on 6cbe5eadd — one clause, no code change. Round-3 cross-review (opus), and it is a claim the reviewer passed in round 1 rather than something this round introduced.

pcapkit/corekit/enum.py:284-287 now says the _value2member_map_ restriction "keeps this method's own 'never mints' claim true for every str-valued registry, not only the ones without a minting _missing_." That is false. The str branch's own return cls(default) at :297 reaches _missing_ and mints. Scope the clause to the value side: the restriction keeps the value path non-minting; it does not make get() non-minting.

I verified it and it is wider than the review said. The reviewer scoped this to str-valued registries, which are unreachable today. The int path at :303 is the same cls(default) and is live on a shipped registry right now, measured on main (6b333d7e4), throwaway process, tree asserted:

len before      : 160
EtherType.get(0x1234, 0x0888) -> <EtherType.Xyplex_0x0888: 2184>
len after       : 161
MINTED          : ['Xyplex_0x0888']

So :267's bare "It never mints." is false today, not merely after #860 step 2. Filed as #864 with needs: decision, because whether a caller who names a fallback has "explicitly" asked for that member is a ruling, not a measurement. Pre-existing — cls(default) is identical on main before this PR — so out of scope here; #863 only needs its own new clause narrowed.

Everything else in round 3 confirmed, and the "prose only" claim was proved mechanically rather than taken from the diff: both files parsed at both revisions, docstrings stripped, ASTs identical, and the non-docstring string constants identical too (38 in enum.py, 221 in the test file) — so no assertion and no code changed. The reworded "verbatim" claim is now accurate on both counts. The absolute "the non-str path has no such gap" survived a direct attack: a registry minting for anything still cannot be reached that way, because aenum itself refuses — TypeError: values must be str [b'raw' is a <class 'bytes'>]. 69 tests pass under plain unittest.

One non-blocking suggestion from the review I agree with: add a five-word qualifier to the unconditional promise at :270-273 too, since that is the sentence a reader trusts.

[JarryShaw]

Round 3 pushed — head e36ab4d77, same single commit amended in place, no rebase needed. enum.py now +33/−1, still prose only.

The clause is correctly scoped now (:285-291, verified by me at the pushed sha): "Restricting the value side of key to an already-registered value keeps that side non-minting on every str-valued registry … it is a claim about the value side alone, not about this method as a whole: an unregistered default still reaches cls(default) below exactly as it already did, and can mint there just the same." The author reproduced the minting independently before editing.

The optional qualifier was taken too — :273-274 now ends "deliberately absent from the lookup tables -- for a non-str key; the str case is qualified below", which is the −1 in the diffstat.

:267's bare "It never mints." and cls(default) are both untouched, deliberately: that is #864's territory and needs a ruling, not a prose patch. 69 tests still pass under plain unittest.

Label back to review: pending; re-review dispatched on this head.

[JarryShaw]

GOOD TO GO — round-4 cross-review (opus, author sonnet) on e36ab4d77, plus its two non-blocking nits, which I took myself. New head 216369e28, same single commit, enum.py +34/−2, prose only.

The corrected clause discharges the round-3 finding. The reviewer ran the "exactly as it already did" half on both trees, five cases diffed, and four are byte-identical — including case A, the one the clause is about, which mints on both trees. So "can mint there just the same" is confirmed against main, not just the current tree.

Its section-2 reasoning went stale mid-flight, in a way that strengthens the verdict. It argued that fixing :267 here would pre-empt #864's ruling and that "if it rules the other way, the fix is in cls(default), not in the docstring". That ruling landed at 22:05:30Z while the review was running — "Take (b). Only register can mint." — so the direction is settled and the fix does belong in cls(default), under #864. #863 stays prose-only, which is what the reviewer concluded for reasons that now hold more firmly than when it wrote them.

The two nits, both taken:

• "exactly as it already did" was not literally true: the reachability of cls(default) shrank by exactly one input class — where key is a registered value, the new value match pre-empts the default (case B, minted=['BRAND-NEW-DEFAULT'] on main vs minted=none on head). Three words dropped. The review declined to block on it with a stated rule rather than a mood: rounds 1–3's errors would have made a reader under-estimate the hazard, while this one makes them over-estimate it, and nobody is burned by being too careful.
• :267 now reads "It never mints -- see EnumRegistry.get() mints through its default, contradicting its own "It never mints" contract #864 for the default path, which does and should not." The review offered this as optional while EnumRegistry.get() mints through its default, contradicting its own "It never mints" contract #864 was open; with the ruling landed it is a pointer to a decided defect, not speculation.

"Prose only" proved mechanically again, not read off the diff: both revisions parsed, docstrings stripped, ASTs identical, non-docstring string constants identical (38 in enum.py, 221 in the test file), and git diff e36ab4d77..HEAD -- tests/ empty. 69 tests pass under plain unittest at my amended head. Four rounds, no repo file edited by any reviewer, every probe in /tmp.