feat(reg)!: retype AppType.proto to its own transport, and give register_apptype varargs (#806)

[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 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
• feat — adds a feature — breaking, feat(reg)!:
• perf — changes performance, not behaviour
• refactor — changes neither behaviour nor performance
• test — tests only
• docs — documentation only
• ci — workflows or build tooling
• chore — anything else

---

Description of your pull request and other information

Closes #806. Closes #809. Step 1 of #801's three, per the ruling recorded there.

AppType.proto held the whole transport protocol set IANA assigned the service, so TCP['http'].proto named UDP and SCTP as well. That was a second copy of what the four per-transport registries already encode — and the copy that could drift. It is now the single transport of the registry the member lives in. Done in the generated source rather than at runtime: proto.name is folded into the member's underlying str value by __new__, and that value is the live key in _value2member_map_, so 10,625 of 12,391 members change .value, repr() and str().

The two halves are one change because register_apptype relied on the multi-bit value to fan out: it read proto = code.proto and tested each bit with in, so register_apptype(TCP.http, Dummy) displaced the UDP handler for port 80 too. It now takes *transport varargs, named one at a time, with class_ keyword-only — a positional-with-default ahead of the varargs binds the first transport to class_ and silently eats it, which tests/const/test_const_apptype_split_unit.py now pins with an inspect.signature().bind() check. A member with no explicit transport uses its own (now single-bit) proto; a bare int with none is a RegistryError, as is a hand-built composite.

Not regenerated — AppType crawls IANA's live CSV, so a regeneration is not reproducible. The generator and its output were hand-edited, then BASE(...)/TRANSPORT(...) re-rendered from arguments extracted out of the committed const files, reproducing all five byte-identically (and origin/main's five from its own templates, as a control). Also drops the stale consider-using-f-string disable the TRANSPORT template emitted into the four registries, which is #809.

[JarryShaw]

Cross-review verdict: GOOD TO GO (sonnet; author was opus). No unprompted findings — an unusually clean change. I re-derived the two decisive claims myself, with a self-test so the zeros mean something:

PROVENANCE: /tmp/w815/pcapkit/__init__.py
members=12391  multi-bit=0  empty=0  proto!=own_registry=0  value-formula-mismatch=0
per-registry: TCP 6147  UDP 6143  SCTP 91  DCCP 10
TCP["http"]: <TCP.http: 80 [tcp]>   value = 'http [80 - tcp]'
SELF-TEST (a deliberately wrong formula must mismatch): 91 of 91

The signature binds correctly and class_ cannot be eaten:

code POSITIONAL_OR_KEYWORD | module POSITIONAL_OR_KEYWORD | transport VAR_POSITIONAL | class_ KEYWORD_ONLY
bind(code, module, TP.udp) -> transport=(udp,)   class_ bound? False
old proto= kwarg          -> TypeError: unexpected keyword argument 'proto'     (removed, not silently accepted)
pickle / deepcopy identity -> True / True

Not regenerated — confirmed by a stronger method than I asked for, and the substitution was flagged. Rather than re-invoking BASE, the reviewer classified every +/- line of the four registry diffs against the three expected patterns (two docstring rewords, the disable drop, and single-line TransportProtocol.x | y → TransportProtocol.<own> flips) and found zero unaccounted lines in any file — 100% coverage rather than a sampled render-and-diff. Member counts identical at 12,391 pre and post, and the vendor generator's split predicate if self.TRANSPORT not in record.protos or record.port == '-1': is byte-unchanged. Classifier self-tested against an injected multi-bit line (flagged) and a correct single-bit line (not flagged). It also independently reproduced multibit_main_count == 10625.

The tests were rewritten, not weakened — each one checked: :80's assertEqual(TCP.http.proto, UDP.http.proto) → assertNotEqual plus two assertIs(..., cls.__transport__) pins; test_every_multi_transport_member_refuses_its_own_proto (which asserted swept == 10625) replaced by test_no_member_carries_a_multi_transport_proto asserting the inverse over the full population and re-deriving the same 23 (port, svc) pairs and the same two divergent rows at 888/999 against the new behaviour; and test_no_lookup_call_site_in_the_library_builds_a_composite_proto now assertNotIns the two old literals, assertIns the new varargs signature, and runs a live inspect.signature/.bind() check of the same shape I ran independently. That last one was the deletion risk and it was rewritten. tests/foundation/registry/test_protocols.py 10/10 under plain unittest.

#809 is genuinely folded in, verified per file:

const/reg/apptype/{tcp,udp,sctp,dccp}.py   disable=0  %-fmt=0
vendor/reg/apptype/apptype.py              disable=2  %-fmt=6   <- earned

The two remaining per-line disables in the generator sit on real % sites (' :widths: %s' % … and another %-join), so they are correct and correctly untouched.

The reviewer caught its own probe artefact: its first byte-identity pass used str(proto) instead of proto.name and produced 12,391 false mismatches — caught by re-deriving the formula rather than trusting the first run. Thirteenth artefact this session and the fifth caught by the agent that made it.

UNVERIFIED: the coverage delta, dropped for budget after the exhaustive member-level checks. The rewritten tests read as a net increase in assertions but that was not measured.

One housekeeping note: the reviewer's worktree could not be returned to its original branch, because fix/800-http-version-positive-identification is checked out in #814's worktree. Its tree is clean and it made no edits; I will clear both worktrees once these merge.

Flipping to review: good-to-go.

[JarryShaw]

One thing needs your ruling before this merges, and the author flagged it rather than deciding it. Adding needs: decision; the review: good-to-go stands, since the code correctly implements what you ruled.

register_apptype no longer accepts a transport name, and the failure is not clean. The old form took proto: 'TransportProtocol | str' and coerced at protocols.py:839-840 (if isinstance(proto, str): proto = TransportProtocol.get(proto.lower())); the repo's own test used it at test_protocols.py:493 as proto='tcp'. At this head:

register_apptype(65432, Raw, 'tcp')  ->  AttributeError: 'str' object has no attribute 'name'

Two separable questions:

1. Should names still be accepted? Your ruling gave the annotation as *transport: TransportProtocol, and the author kept it verbatim — correctly, since mypy.ini sets warn_unreachable = True, so an isinstance(t, str) branch under that annotation is a mypy error. Accepting names again means widening to *transport: 'TransportProtocol | str'. That is a change to your own ruling, so it is yours.
2. Either way, the AttributeError is a defect. A string reaching this function should raise something a caller can act on — a RegistryError naming the argument — not an AttributeError from deep inside. That holds whether you accept names or reject them, so I would fix it regardless.

My recommendation: reject names, but reject them properly. It matches the "singular and explicit" principle you gave, and TransportProtocol.tcp is no harder to type than 'tcp'. But a caller upgrading from the old kwarg deserves a diagnosis rather than an attribute error.

Also flagged by the author, and worth its own issue rather than holding this PR: _missing_ still mints span members carrying TransportProtocol.undefined (const/reg/apptype/apptype.py:2272, :2296, :2427) rather than the registry's own transport. Pre-existing, so "every member names its own registry" is proven for declared members only. Filing separately.

One correction to my own brief: I told the author tests/const/test_const_apptype_split_unit.py was 863 lines / 19 methods. It is 923 lines / 20 methods on 4530424df — #803 grew it after I measured. The author caught this and re-derived everything itself, which is the right response to a stale number in a brief.

And the author's own honesty note is worth keeping: its first post-change coverage run read 63% with protocols.py dropping 89% → 68%, and it did not reproduce — three isolated runs give 89% / 27 missing either side. It treated the first run as a flake only after failing to reproduce it, rather than instead of trying. It also ran ~75 minutes against a ~25-minute budget and said so, with the tree complete rather than half-applied.

[JarryShaw]

I think we can accept the str value and coerce it into TransportProtocol enumeration. This is a light lift.

[JarryShaw]

Ruling recorded, in your words:

I think we can accept the str value and coerce it into TransportProtocol enumeration. This is a light lift.

So *transport accepts str as well as a TransportProtocol member and coerces. Concretely, this PR currently narrows the parameter from the old proto: 'TransportProtocol | str' to *transport: 'TransportProtocol' — dropping str entirely — and that narrowing is what your ruling reverses. The four @overload signatures at pcapkit/foundation/registry/protocols.py:786-793 and the runtime signature at :796 all need 'TransportProtocol | str' back, with coercion at the top of the body and the docstring's "rather than as its name" clause corrected.

needs: decision is cleared and this is review: needs-changes until that lands — the previous good-to-go was against the str-rejecting behaviour, so it no longer describes the PR you want. A worker is on it; class_ stays keyword-only, which is unrelated to the ruling and load-bearing (a positional default ahead of *transport swallows the first transport).

One thing the coercion has to decide, and I will make it strict unless you say otherwise: a name that does not match any member, and a name like "tcp|udp", should raise RegistryError rather than silently resolve — same as the member path already does for a composite, since one call registers under one transport.

[JarryShaw]

Your ruling is implemented at e683be072 — *transport now takes a TransportProtocol member or its name as a str, coerced before any registry work. Overloads 786-793, runtime signature 796-797, docstring 812-834, coercion at 852-869. Two files: protocols.py and tests/foundation/registry/test_protocols.py.

The author found a real trap, and I reproduced it rather than take its word. The obvious coercion, TransportProtocol[name], would have silently defeated the composite refusal — aenum's Flag.__getitem__ parses a '|'-joined name into a composite:

TransportProtocol['tcp|udp']        -> <TransportProtocol.tcp|udp: 3>     # silent
TransportProtocol.__members__['tcp|udp'] -> KeyError                      # what it uses

So it goes through __members__ directly, case-sensitively. An unknown name and a composite both raise RegistryError('unknown transport protocol: …'), matching the wording the member path already uses — a str and its member are indistinguishable to a caller, including in failure.

It also correctly declined to reuse the existing TransportProtocol.get(), and I confirmed why: get() mints an unknown name rather than raising —

TransportProtocol.get('bogus2') -> <TransportProtocol.bogus2: 16>   # members 5 -> 6

which is exactly the behaviour #775 exists to remove, and the opposite of a strict coercion.

Tests: 6 new blocks (single str, several str, str+member mix, unknown name, composite, and a str-vs-member equivalence check on mock.call_args), each shown failing on 4c035d2c4 with AttributeError: 'str' object has no attribute 'name' and passing after. tests/foundation regression: 193 tests, 11 skipped, all passing. Coverage held at 89% with absolute covered lines up (277→286 stmts).

Label is review: pending — the head moved, so the earlier verdict no longer applies. Cross-review dispatched on a different model.

[JarryShaw]

Cross-review verdict: NEEDS CHANGES (opus; author was sonnet). I re-ran the load-bearing probes against origin/main and this head side by side, and they change the picture in both directions.

1. This PR removes a defect that ships today. main mints an enum member for every bad name — measured, __members__ 5 → 9:

main:  'tcp ' -> RegistryError, members 5->6      PR: members 5->5
       'bogus'                         6->7                    5->5
       'tcp|udp'                       7->8                    5->5
       ''                              8->9                    5->5
  main final members: [... 'tcp ', 'bogus', 'tcp|udp', '']

That is the #775 class, live on main, and the coercion via __members__ removes it. Worth saying in the PR description.

2. Required: format the rejected name with !r. 'tcp ' currently reports unknown transport protocol: tcp — the trailing space is invisible, so it asserts tcp is unknown when tcp is the one name that is known. {proto!r} at protocols.py:865. Safe: it only ever formats an unresolvable name, which has no member counterpart, so the str/member message equivalence is untouched.

3. Required, and I am correcting the reviewer here: the int/bool half-write is a regression against main, not pre-existing. It called it pre-existing having checked 4c035d2c4 — but that is this PR's own earlier head, which already carried the varargs change. Against main:

main:  register_apptype(p, Unit, proto=1)    -> TypeError, wrote NOTHING
PR:    register_apptype(p, Unit, 1)          -> AttributeError, wrote [9706]  <-- port registered, then crash

Because TransportProtocol is an IntFlag, registries.get(1) finds TCP, TCP.register runs, and the next line's proto.name explodes — leaving the port in TCP.__proto__ with register_protocol never run. isinstance(proto, str) should be an else-branch that raises RegistryError, not a fall-through.

4. One thing needs your ruling — see the separate comment below. The coercion is case-sensitive; main is case-insensitive.

Confirmed and not in dispute: no str/member divergence across 11 adversarial pairs measured on real registry state (including the composite, whose .name really is 'tcp|udp' so both paths give the same message); nothing in the new path reaches get()/_missing_/extend_enum (spies recorded zero calls, self-tested); class_ still keyword-only and the only keyword-only parameter; delta scope exactly two files; coverage 89% → 89% with all 9 new statements and 6 new branches covered.

Two testing caveats worth fixing eventually: the six new blocks are sequential inside one test method, so the method dies at the first and the other five never run — "all six fail without the fix" is true per-block but not provable from one invocation. And the equivalence block patches only TCP.register and compares call_args, so it would pass even if the member branch had also written to the real UDP registry.

[JarryShaw]

Needs your ruling: the coercion is case-sensitive, and main is not. That is a narrowing your ruling did not ask for.

Measured side by side, origin/main (21e9588af) vs this head (e683be072):

main:  register_apptype(9700, Unit, proto='TCP') -> OK, registered under TCP
       register_apptype(9701, Unit, proto='Tcp') -> OK, registered under TCP
PR:    register_apptype(9700, Unit, 'TCP')       -> RegistryError: unknown transport protocol: TCP
       register_apptype(9701, Unit, 'Tcp')       -> RegistryError: unknown transport protocol: Tcp

main does TransportProtocol.get(proto.lower()), so it has accepted any casing. A caller who has been passing 'TCP' gets a hard error after this merges. Your words were "accept the str value and coerce it into TransportProtocol enumeration", which does not settle casing either way.

(a) Keep it case-sensitive — exact member names only, as implemented. Documented in the docstring, and the PR is already feat(reg)!, so it goes in the breaking-change note next to the class_/varargs change.

(b) Restore case-insensitivity — one line, __members__.get(proto) → __members__.get(proto.lower()). I checked this still refuses the composite, since 'tcp|udp'.lower() is not a member name either, so the strictness you get from __members__ is preserved.

I lean (b): it is what callers have today, the ruling was framed as "a light lift" rather than a tightening, and it costs nothing in strictness. But it is your API.

Related, and probably the same decision: AppType._dispatch (pcapkit/const/reg/apptype/apptype.py:2380) still does TransportProtocol.get(proto.lower()) for a str. So after this merges, AppType.get(port, proto='TCP') would work while register_apptype(port, mod, 'TCP') would not — and AppType.get would still mint on a bad name. Two public APIs taking "a member or its name" disagreeing on both casing and minting. Whichever way you rule, it should apply to both.

The two required changes from the cross-review (!r on the rejected name, and rejecting non-str/non-member instead of half-registering) do not depend on this answer, so a worker is doing those now.

[JarryShaw]

Your ruling is implemented at ee51366a6, and I verified all five cases plus the residual.

signature: (code, module, class_='(null)', *transport)        <- positional again, sibling convention restored

row1  class module + 1 member            -> tcp=[9401]
row2  class module + 2 members           -> tcp=[9402] udp=[9402]      <- order preserved, prepended
row3  class module + str transport 'udp' -> udp=[9403]
row4  str module + 'Raw' + tcp           -> tcp=[9404]
row5  str module + 'Raw' + udp (4th pos) -> udp=[9405]
str module + 'tcp' as class name         -> AttributeError: module '…misc.raw' has no attribute 'tcp'

That last line is the one that matters: 'tcp' was resolved as a class name, not sniffed as a transport — the residual I flagged as resolved-by-fiat behaves as documented.

The branch is four lines at the top of the body, keyed on type(module) only:

if not isinstance(module, str) and class_ is not NULL:
    transport = (cast('TransportProtocol | str', class_),) + transport
    class_ = NULL

A precision worth recording, because it changes how the test can be written: Python's argument binding is purely syntactic, so a third positional always raw-binds to class_ regardless of module's type. The disambiguation is therefore a body-level effect, not a binding-level one — which means an inspect.signature().bind() assertion cannot express it. The rewritten const test at test_const_apptype_split_unit.py does both halves: bind() showing the third positional lands in class_ unconditionally, and a real call checking UDP.__proto__[65210] to show the body redirects it. Still all on the live signature and function object, never on source text.

It also caught and fixed a test from round 1 that had relied on the now-reversed convention — register_apptype(65000, mod, TransportProtocol.tcp, class_=raw) is a TypeError: got multiple values for argument 'class_' under the new signature. And it swept the whole package for other register_apptype( call sites: none outside protocols.py and the two test files, and the .rst references are autofunction/prose rather than literal signatures.

tests/const/test_const_apptype_split_unit.py 21 OK, tests/foundation/registry 15 OK, 31 under pytest with no subtest miscount. Coverage 89% held, 288→291 statements, same missed set. aliases not added, per your second ruling.

Label is review: pending — the four earlier verdicts were against the keyword-only signature and cannot carry across a deliberate reversal. A delta re-review is dispatched.

[JarryShaw]

Delta verdict: GOOD TO GO (opus, same reviewer). Nothing blocking. It also corrected my brief twice, both verified.

My diff instruction was wrong. I told it ee51366a6 and c55a83c3b share parent 4530424df; the real parent is c287152d7 — my own rebase at 02:48Z. So the two-dot diff I specified reports 36 files / 1930 insertions, mostly main-side movement, and lists transport/{tcp,udp}.py, dispatch.py and test_http_unit.py — all off-limits. Those are rebase artefacts only: the PR's own file set against its parent is the same nine files as round 1, and a grep for the off-limits paths in that set returns NONE. Pure delta on the three changed files is 155 insertions / 31 deletions; the regenerated const/reg/apptype/* and vendor/reg/apptype/* tables are unchanged by this round.

Prepend-not-append proven by asymmetry rather than by reading the code, which is the nicer proof: (Unit, TP.tcp, TP.sctp) writes TCP then raises on sctp; (Unit, TP.sctp, TP.tcp) raises with nothing written. Only prepending produces that ordering.

Direct answer to the question I posed: a class module with class_ as a keyword does not silently register under no transport — it raises. But there is one corner worth putting in the PR description rather than only the docstring, and I reproduced it:

register_apptype(port, Unit, class_='x')   -> RegistryError: unknown transport protocol: 'x'   wrote=[]
register_apptype(port, Unit, class_='tcp') -> OK                                               wrote=[9501]

The branch never sniffs the value — it keys on type(module) exactly as you ruled — but the observable outcome of an explicit class_= with a non-str module depends on whether the string happens to name a transport. class_= with a class module is a caller error under any reading, so not a defect.

On the bare AttributeError for the str-module residual: leave it, and the reasoning is stronger than "out of scope". It comes from ModuleDescriptor.klass (pcapkit/corekit/module.py:37), and protocols.py builds ModuleDescriptor(module, class_) in nine places — every register_* sibling fails the same way for a bad class name, demonstrated on this same head by E_TCP.http, '…raw' with class_ omitted giving AttributeError: … has no attribute '(null)'. Fixing it only here would make register_apptype inconsistent with its eight siblings. Filing it as a family-wide follow-up.

What this delta does change is reachability: under the keyword-only shape f(port, 'a.b', 'tcp') bound 'tcp' to *transport and registered under TCP; now it binds to class_ and dies. Same call, new meaning — which is the point of your ruling, but worth stating.

Both rewritten tests have teeth against the pre-reversal source, checked on a scratch tree at c55a83c3b: the registry test fails with RegistryError: unknown transport protocol: 'Raw', and the const test fails on the parameter-kind list (KEYWORD_ONLY last vs POSITIONAL_OR_KEYWORD third). Each fails on exactly what the reversal changed. And the bind() assertion now pins the undesired outcome on purpose — recording that pure syntax does swallow, with the real registry call proving the body undoes it.

Nothing from earlier rounds regressed: .lower() case-insensitivity, the !r, composites, unknown names, and the elif guard that closed the IntFlag half-write — 1, True, b'tcp' all raise with nothing written to either registry, which is the one the reshuffle could plausibly have broken since class_ is now prepended before the coercion loop. __members__ flat at 5 throughout. 36 passed / 38163 subtests under pytest, 31 OK under plain unittest.

[JarryShaw]

For the NULL value in the foundation registry, can we actually use a sentinel rather than a pure string? This is not in scope of this PR tho. File an issue and we can talk further.