register_protocol's key space is still not unique: the three HTTP classes share one key (follow-up to #675)

Follow-up to #675, which is fixed by #681. #675 was about the silence: register_protocol
displaced one protocol class with another and said nothing. #681 makes the displacement audible
with a RegistryWarning. It deliberately does not resolve the collision, and this issue is
the residue.

What is still true after #681

pcapkit.protocols.__proto__ is keyed on cls.__name__.upper(), and three dispatchable classes
are all named HTTP:

pcapkit/protocols/application/http.py — the generic base HTTP
pcapkit/protocols/application/httpv1.py — HTTP
pcapkit/protocols/application/httpv2.py — HTTP

All three still land on the single key 'HTTP'. Whoever registers last still wins; the only
difference is that you now get told. Measured on b34f132f6:

distinct keys the three classes land on: ['HTTP']

The import-time seeding at pcapkit/protocols/__init__.py:74-75 avoids the clash only because it
keys off the distinct __all__ names HTTP, HTTPv1, HTTPv2 rather than off cls.__name__.
Anything going through register_protocol — including Protocol.__init_subclass__, which
registers unconditionally — gets the collision.

Why #681 stopped at the warning: every reader takes a bare name and fails silently

This is the part worth not re-deriving. A unique key (qualified name, or the protocol's own
declared identifier) is a registry-format change, and not one reader raises on a miss — they
all degrade quietly, which means a re-keying would be a second silent behaviour change rather
than a fix.

Reader	Key it looks up	Behaviour on a miss today
`ProtocolBase.expand_comp` — `pcapkit/protocols/protocol.py:697`	`value.upper()` from a caller-supplied string	Falls back to `comp = (value.upper(),)`, a bare string with no class identity
`ProtocolBase.__getitem__` / `__contains__` / `_check_term_threshold`	via `expand_comp`	Inherits the above — this is `packet['HTTP']`, `'HTTP' in packet`, `extract(protocol=...)`
`ProtoChain.index` / `count` / `__contains__` — `pcapkit/corekit/protochain.py:113,133,207`	via `expand_comp`	Inherits the above
`ReassemblyMeta.protocol` — `pcapkit/foundation/reassembly/reassembly.py:77`	`cls.name.upper()`	Returns `Raw`
`TraceFlowMeta.protocol` — `pcapkit/foundation/traceflow/traceflow.py:78`	`cls.name.upper()`	Returns `Raw`
`PayloadField.protocol` setter — `pcapkit/corekit/fields/misc.py:267`	`protocol` verbatim, no `.upper()`	`None`, then falls back to `Raw`

Two consequences that make this more than a mechanical rename:

expand_comp is handed a bare name by construction. It takes a user string like 'HTTP'.
A qualified key does not relocate that lookup, it removes the ability to perform it. So
re-keying needs a deliberate answer for "how does a user still say packet['HTTP']" — most
likely a second, name-to-key index, which is a design decision rather than an edit.
It breaks alias classes today if done naively. IPsec.id() returns ('AH', 'ESP') —
'IPSEC' is not in its own id(). frame['IPsec'] works only because the registry hit
resolves the class and the comparison then uses id(). On a miss it would raise
ProtocolNotFound instead of matching an ESP layer.

pcapkit.protocols.__proto__ is also a documented public attribute —
docs/source/pcapkit/protocols/index.rst:154 documents it with autodata, cross-referenced to
register_protocol — so its key shape is part of the public contract, not an internal detail.

Options, not a prescription

Qualified key plus a bare-name index. Key on f'{cls.__module__}.{cls.__qualname__}'.upper()
and keep a separate name-to-entries map for expand_comp, which then has a defined answer for
an ambiguous bare name (first hit, last hit, or refuse and say which candidates exist).
Key on the protocol's own declared identifier rather than its Python class name. id()
already exists and is already what the comparison path uses, which makes it the more principled
key. Needs a survey: id() returns a tuple for several protocols, and IPsec shows the class's
own name need not appear in it.
Rename the classes so the key space is naturally unique. Smallest conceptual change, largest
API break — pcapkit.protocols.application.httpv2.HTTP is importable today.

Relationship to #514

#514 is the design issue for adopting the opt-in EnumMeta/EnumSchema registration pattern
across Protocol, Engine, Reassembly and TraceFlow; #575 is gated behind it. Its part-(c)
investigation recommended fixing this collision first, as c1, before the alias work — #681 is
that c1 step, and it was scoped to observability precisely so that it constrains nothing about the
eventual key. Whichever option above is taken belongs with #514's decision rather than ahead of it,
since #514 is what decides whether registration is opt-in at all, and an opt-in registry has a
materially smaller key space to keep unique.

Worth recording from #514's own measurements, because it bears on how urgent this is: Protocol
has 0 descendants and ProtocolBase has 43, so no built-in currently reaches the
collision through __init_subclass__. Verified on b34f132f6 — all three HTTP classes derive
from ProtocolBase, not from the public Protocol, and a plain import pcapkit emits zero
RegistryWarnings. The collision is reachable today by an explicit register_protocol call, or by
user code subclassing the public Protocol under a name a built-in already holds. That is a real
but narrow blast radius, which is the argument for doing this with #514 rather than in a hurry.

Needed from you: nothing yet — this is gated on #514

Recording the dependency explicitly so it does not read as awaiting input. This issue's own body settles it: "Whichever option above is taken belongs with #514's decision rather than ahead of it, since #514 is what decides whether registration is opt-in at all, and an opt-in registry has a different eventual key."

So the sequence is #514 decides → this one becomes decidable. Labelled blocked accordingly rather than left looking like an open question for you.

What is already settled here and needs no revisiting, so it is not re-derived later: all three of application/http.py, httpv1.py and httpv2.py land on the single key 'HTTP' because pkcapkit.protocols.__proto__ keys on cls.__name__.upper(); last registrant wins; #681 made that audible with a RegistryWarning but deliberately did not resolve it. The reason a re-keying was not simply done: not one reader raises on a miss — expand_comp (pcapkit/protocols/protocol.py:697) falls back to a bare string, ReassemblyMeta.protocol and TraceFlowMeta.protocol return Raw, and PayloadField.protocol (pcapkit/corekit/fields/misc.py:267) looks up verbatim with no .upper() and falls to Raw. So changing the key would be a second silent behaviour change, not a fix.

The import-time seeding at pcapkit/protocols/__init__.py:74-75 escapes the clash only because it keys off the distinct __all__ names.

Unblocking: the dependency was #514, which is now closed (by #752). But it closed with "nothing added" — the maintainer ruled the custom-versus-builtin registration check is not wanted — so #514 did not answer this issue's question, it removed the thing this issue was waiting for.

So the key space is still not unique, and the residual risk recorded on #514 stands verbatim: the registry is keyed on __name__.upper() (foundation/registry/protocols.py:219), so a user class named HTTP still displaces the built-in. Narrower than the three-way collision in this body, not zero.

Newly relevant: a measurement taken for #514's blast radius found HTTP appears in both the leaf and non-leaf descendant lists of ProtocolBase — two distinct HTTP classes are live in the tree today, which is this issue's collision surface rather than a hypothetical.

Also worth recording before anyone picks this up: #514's ruling (b) — collapse each XXXBase/public pair and make name-registration opt-in by keyword — would make this displacement explicit rather than automatic, since a built-in simply would not pass the keyword. That is the cheapest available fix for this issue and it is already decided in principle, so this should probably be sequenced behind that work rather than solved independently.

Blocked on #778, which I have just filed so the blocker is checkable rather than a reference to a closed issue's comment thread.

The chain: this issue's cheapest fix is #514's ruling (b) — collapse each XXXBase/public pair and make name-registration opt-in by keyword, which turns the __name__.upper() displacement from automatic into explicit, since a built-in simply would not pass the keyword. But #514 closed with "nothing added", so (b) has no tracking item and no confirmed go-ahead. #778 carries the design, the ≈390-edit-point blast radius, the three hazards no issubclass sweep finds, and the one question that decides whether it happens at all.

Checkable blocker: #778 is answered and, if wanted, its opt-in-by-keyword registration has landed. If #778 comes back "dropped", this issue closes recording the displacement as accepted residual risk instead.

Needs your decision, and I have asked twice without an answer — so labelling it rather than leaving it in a comment thread.

The ask: does #778's @final runtime guard dispose of this issue, or does #682 wait for #514's ruling (b)?

Where it came from. On #778 you said "the custom/builtin check part has been moved to the @final implementation i believe" — the "i believe" hedged, and I could not make it reach this issue. Then you clarified the mechanism: "check typing.final's added __final__ dunder value in __init_subclass and to raise when subclassing the @final class." That clarification is what makes me fairly confident the two do not meet, because:

This issue's defect is not a subclassing event. pcapkit/foundation/registry/protocols.py:219 keys the registry on protocol.__name__.upper(). A user class named HTTP displaces the built-in by name collision at registration time. __init_subclass__ never fires for that, so no __final__ check on any base can see it.
:217 already raises RegistryError for a non-ProtocolBase argument, and that is the only custom-versus-builtin gate on that path today.
The @final machinery lives in pcapkit/corekit/infoclass.py and pcapkit/protocols/schema/schema.py and governs Info/Schema finalisation — a different mechanism from the protocol registry entirely.

Three ways this can go, and each changes what I do next:

register_protocol's key space is still not unique: the three HTTP classes share one key (follow-up to #675) #682 waits for (b) — opt-in-by-keyword registration makes the displacement explicit, since a built-in would not pass the keyword. This is the reading I have been working to, and it is why register_protocol's key space is still not unique: the three HTTP classes share one key (follow-up to #675) #682 currently carries blocked.
The @final does cover it and I am missing the connection — tell me where, and I will look again rather than argue.
Neither — the __name__.upper() displacement is accepted residual risk, and register_protocol's key space is still not unique: the three HTTP classes share one key (follow-up to #675) #682 closes with that recorded.

Nothing else is waiting on this, so it is a quiet hold rather than a stall: #775, #766, #732 and #719 all have their own separate blockers, and the @final work itself is in flight regardless.

I originally designed the abstract HTTP class as a proxy to dispatch the actual HTTP/1 or HTTP/2 protocol parsing. But if it's causing issue here, we should review if both protocols are bound to the same port or different. And then decide how to move forward. This is something that might apply to all other versioned protocols sharing same namespace/enum.

Recorded, in your words: "I originally designed the abstract HTTP class as a proxy to dispatch the actual HTTP/1 or HTTP/2 protocol parsing. But if it's causing issue here, we should review if both protocols are bound to the same port or different. And then decide how to move forward. This is something that might apply to all other versioned protocols sharing same namespace/enum."

So the answer is none of my three options — it is investigate first, then decide, and the two live HTTP classes are deliberate design rather than the accident I read them as. Taking needs: decision off; it goes back on when I have the findings and can put concrete options in front of you rather than a question.

What I can already see, so the investigation starts from facts. The name HTTP is on six classes, not two:

pcapkit/protocols/application/http.py:32 — class HTTP(Application[_PT, _ST], Generic[_PT, _ST]), the proxy you describe, whose docstring names both HTTP/1.1 (RFC 7230) and HTTP/2 (RFC 7540)
pcapkit/protocols/application/httpv2.py:96 — class HTTP(HTTPBase[Data_HTTP, Schema_HTTP]), the concrete HTTP/2
plus two Data classes (data/application/httpv1.py:27, httpv2.py:42) and two Schema classes (schema/application/httpv1.py:14, httpv2.py:87)

The two that matter for this issue are the first pair, since those are the ProtocolBase descendants the __name__.upper() registry keys on. And pcapkit/protocols/application/http.py contains no port registration at all, which is itself worth following: if the proxy never registers a port, the displacement may be latent rather than active, and that changes how urgent this is.

Dispatching the review you asked for, scoped to your three questions: whether HTTP/1 and HTTP/2 bind the same port or different ones; what the proxy's dispatch actually does at parse time and whether registration reaches it; and the sweep you flagged as the real point — which other versioned protocols share a namespace or enum the same way. I will come back with the port facts, the full list of affected families, and options costed against each.

Investigation done. Your three questions answered, and the third one settles the scope decisively.

Same port — but the split is by transport, not by version. tcp.py:330-331 binds 80 and 8080 to the concrete HTTP/1 (application/httpv1.py:84); udp.py:102-103 binds the same two ports to the proxy (application/http.py:32). 443 is deliberately absent on both (tcp.py:325-327: IANA pcsync-https, TLS unparsed). Sweeping _lookup_next_layer over all 65536 ports on both transports: httpv2.HTTP is the value of no port on any transport. udp.py:67-76 already documents that TCP/UDP asymmetry as knowingly left alone.

The collision is latent, not active. pcapkit.protocols.__proto__ holds 38 keys and all three classes are distinctly addressable — HTTP → the proxy, HTTPV1, HTTPV2 — with no RegistryWarning on import pcapkit. The reason: the only auto-registration hook is Protocol.__init_subclass__ → register_protocol (protocol.py:1924), and Protocol.__subclasses__() is empty — every built-in descends from ProtocolBase directly, so that hook is a user-extension path only. A user class named HTTP does displace, warning as it goes, and :216-217's RegistryError for a non-ProtocolBase argument is the only gate.

Q3, the one you flagged as the real point: exactly one collision group exists in the whole tree. 43 ProtocolBase descendants, one group with more than one member — HTTP. Six proxy families exist (ip.IP, ipsec.IPsec, l2tp.L2TP, vlan.VLAN, rarp.RARP and this one) and HTTP is the only one whose versioned subclasses reuse the base's name; the rest already follow the convention HTTP departs from. The 138 other duplicate names in the Info and Schema trees are inert, because __name__.upper() is functional at exactly one place, foundation/registry/protocols.py:219. So "might apply to all other versioned protocols" is true in the namespace sense and empty in the registry sense.

Two live defects found on the way, both independent of how you decide this. PayloadField.protocol (corekit/fields/misc.py:267) does __proto__.get(protocol) with no .upper(), so PayloadField(protocol='http') silently yields Raw today. And _guess_version (http.py:202-207) suppresses only ProtocolError while httpv1.HTTP raises a bare ValueError on HTTP/2 wire bytes, so its HTTP/2 arm is never reached — meaning HTTP/2 is reachable only by explicit version=2, never automatically. Filed as #787.

Needed from you

Five options, costed:

Accept as residual risk — 0 edits. RegistryWarning already fires. Leaves reg['HTTP'] displaceable silently at the point of use.
Rename the concrete classes to HTTPv1/HTTPv2 in their own modules — removes the collision at source, makes the proxy unambiguously 'HTTP', and brings HTTP into line with the other five families. 4 pcapkit files, 3 test files, examples/generators/options.py, plus .rst targets. The HTTPv1/HTTPv2 aliases already exist at application/__init__.py:25-26, so a one-line back-compat alias per module covers the public-name break.
Re-key the registry on module+qualname or an explicit __registry_name__ — 2 writers, 1 seeder, 4 readers. But pcapkit.protocols.__proto__ is documented public and every reader degrades silently on a miss, so the break is invisible where it happens. This is what register_protocol's key space is still not unique: the three HTTP classes share one key (follow-up to #675) #682's body sequenced behind Design: adopt the EnumMeta/EnumSchema opt-in registration pattern for Protocol, Engine, Reassembly and TraceFlow #514, which closed with nothing added.
design: #514 recorded two rulings it never implemented — the Base/public collapse and the enforcing @final #778's opt-in-by-keyword (~390 edit points) — makes displacement explicit, but does not decide which of the three classes owns 'HTTP'.
Fix only misc.py:267 — one line plus a test; fixes the lowercase fallback, does nothing for displacement.

Two things only you can decide: whether TCP:80 should bind HTTP/1 or the proxy (repointing changes what existing captures parse to), and whether any downstream consumer reads __proto__ by bare name, which sets option 3's blast radius.

okay.
yea, let's rename them.
since we renamed the concrete classes, i suppose we dont have to re-key the registry now?
does HTTP/1 and HTTP/2 works on same port? if so, we might then keep just abstract HTTP as an entry point to dispatch. Or, we can introduce a helper function, like the one in pcapkit.foundation.analysis.analyze() to handle this distinguishing process when triggered.
okay.

whether TCP:80 should bind HTTP/1 or the proxy (repointing changes what existing captures parse to)

does HTTP/1 and HTTP/2 works on same port? if so, we might then keep just abstract HTTP as an entry point to dispatch. Or, we can introduce a helper function, like the one in pcapkit.foundation.analysis.analyze() to handle this distinguishing process when triggered.

whether any downstream consumer reads proto by bare name, which sets option 3's blast radius.

maybe we key on FQDN then, to avoid customized class name collision as well.

Recorded, in your words:

Yea mkss and sounds good. We guess on the HTTP version and definitely have to make sure the guesing is correct and making sense.

So the sequence is confirmed, and "the guessing must be correct" is now a requirement rather than a nicety — which changes what the middle step has to be. Measured, the guessing is not correct today, in both directions:

real 24-octet HTTP/2 preface  -> answers version=2, but by misparsing b'PRI' as a frame
                                 header (Length 0x505249 = 5263433) with a ProtocolWarning
b'foo bar baz\r\nX: y\r\n\r\n'  -> answers version=2   <- garbage text classified as HTTP/2

It gets the right answer on the preface for the wrong reason, and the wrong answer on text that is not HTTP at all. Trial-parsing cannot fix that: it asks "did a parser accept this?" when the question is "what is this?".

So the middle step is a positive identification, not a wider net. RFC 9113 §3.4 designed the preface for exactly this — 24 fixed octets, b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n', deliberately a well-formed HTTP/1.1 request line whose method is reserved so an HTTP/1 parser rejects it. A prefix compare cannot false-positive on valid HTTP/1 and needs no parse attempt. I am filing that as its own issue so the sequence is auditable rather than implied.

One limit to state plainly, since "correct" is the bar: the Upgrade: h2c form cannot be made correct in this shape at all. On the wire the upgrade request is HTTP/1.1 and parses as such correctly today; the switch takes effect after the 101, so deciding that later segments on the same 4-tuple are HTTP/2 needs per-connection state keyed on the 4-tuple. _guess_version receives one payload with no flow context. Recognising Upgrade: h2c in a request is possible; acting on it is not, and I would rather say so than let "correct guessing" imply it covers that case.

Full sequence, each step tracked:

httpv2: the frame guard tests the declared length, not the buffer, so a 4-octet frame can report length=16777215 #799 — httpv2's buffer-length guard, so the sub-9 class is uniformly refused. In flight.
The preface test — positive identification in _guess_version. Filing now.
This issue — repoint TCP:80/8080 to the proxy, at which point it is the change that starts working.

Recording the dependency so this issue's own hold is checkable rather than implied.

Checkable blocker: #800 merged. That is step 2 — the positive preface identification, filed just now from your "definitely have to make sure the guesing is correct". The repoint here is step 3 and must not land first, for the reason already measured: _guess_version is entered 0 times across all 1604 fixture frames today, so repointing puts 231 real HTTP/1 frames through a guess path that is known-wrong on non-HTTP input.

gh pr list -R JarryShaw/PyPCAPKit --search "800 in:body" --state all --json number,state,mergedAt

Chain, each link tracked: #799 (buffer-length guard, in flight) → #800 (preface test, blocked on #799) → this issue (repoint TCP:80/8080 to the proxy).

Your decision is unchanged and recorded — the proxy is the target. Adding blocked reflects the sequence, not a reopened question, so wip comes off until #800 lands.

Unblocked — #800 merged via #814 at 23:38:44Z as 3cbdf8999. main is now 3cbdf8999.

The three-step sequence you set out is complete through step 2: #799's buffer-length guard landed via #802, and #800's positive preface identification has just landed via #814. This issue is step 3 — repoint TCP:80/8080 from httpv1 to the proxy.

The measurement that justified the ordering, and which now flips in your favour: _guess_version was entered 0 times across all 1604 fixture frames, because pcapkit/protocols/transport/tcp.py:330-331 binds httpv1.HTTP directly for TCP:80/8080 and no fixture uses UDP:80/8080. That is exactly why repointing had to wait — it would have put 231 real HTTP/1 frames through a guess path that was known-wrong on non-HTTP input. #814 fixed that path, verified on both trees.

Three things #814 established that this work should build on rather than rediscover:

The corpus cannot exercise the guess path at all, confirmed by direct instrumentation rather than inference. So repointing is the change that starts exercising it — and after this lands, those 231 frames become the first real traffic through _guess_version. Protochain over all 23 captures must stay byte-identical, and that is now a meaningful assertion rather than a vacuous one.
Garbage text was already handled before fix(http): identify the HTTP version before parsing, not by trial and error (#800) #814 — fix(http): check the buffer's length, not just the declared one, in HTTP/2's frame guard #802's schema.length > length check closed it, since any text payload's first three ASCII octets declare ≥ 2,105,376. Do not re-credit that as a new property.
Upgrade: h2c and mid-stream segments are deliberately out of scope, with a passing regression test pinning h2c as HTTP/1.1 so a later attempt has to change a test that explains why not. Leave that intact.

Re-derive every line number against origin/main (3cbdf8999) — #802 and #814 both rewrote http.py, and tcp.py:330-331 is from an older measurement.

Dispatching a worker.

fix(tcp): repoint TCP:80/8080 at the HTTP version proxy (#682)

fix(tcp): repoint TCP:80/8080 at the HTTP version proxy (#682) (#821)

docs(changelog): the 1.5.0 entries for #804 and #682, whose code ship…

Navigation Menu

Repository navigation

register_protocol's key space is still not unique: the three HTTP classes share one key (follow-up to #675) #682

What is still true after #681

Why #681 stopped at the warning: every reader takes a bare name and fails silently

Options, not a prescription

Relationship to #514

Needed from you: nothing yet — this is gated on #514

Needed from you

10 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

register_protocol's key space is still not unique: the three HTTP classes share one key (follow-up to #675) #682

Description

What is still true after #681

Why #681 stopped at the warning: every reader takes a bare name and fails silently

Options, not a prescription

Relationship to #514

Activity

JarryShaw commented on Sep 24, 2026

Needed from you: nothing yet — this is gated on #514

JarryShaw commented on Sep 24, 2026

JarryShaw commented on Sep 25, 2026

JarryShaw commented on Sep 25, 2026

JarryShaw commented on Sep 25, 2026

JarryShaw commented on Sep 25, 2026

JarryShaw commented on Sep 25, 2026

Needed from you

JarryShaw commented on Sep 25, 2026

10 remaining items

JarryShaw commented on Sep 25, 2026

JarryShaw commented on Sep 25, 2026

JarryShaw commented on Sep 25, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions