fix(const): refuse a multi-bit proto in AppType._dispatch rather than resolving it LSB-first (#759)

[JarryShaw]

Please follow the guide below

• You will be asked some questions, please read them carefully and answer honestly

• Put an x into all the boxes [ ] relevant to your pull request (like that [x])

• Use Preview tab to see how your pull request will actually look like

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

Tick the commit type your subject line carries.

• fix — corrects a defect
• feat — adds a feature
• 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

Fixes #759. _dispatch resolved a composite proto by its lowest set bit, so any composite containing tcp dispatched into the TCP registry. It now raises ProtocolError from pcapkit.utilities.exceptions — a ValueError subclass, so get/get_all's documented Raises: still holds, and loud rather than quiet because nothing catches it (unlike _missing_'s guard, which ConstEnumBuiltinParityTests.test_the_exception_is_not_an_in_library_one requires to stay a non-BaseError).

Blast radius, re-derived on fe80b8525. Sweeping all 10,625 multi-transport members through get(m.port, proto=m.proto): 0 exceptions, 0 mints, 46 answers naming a service other than the member's own, at 20 distinct ports. Those 46 are three different things, and only the last is a wrong answer: 44 differ only in that the member is not its port's canonical, which a single-bit lookup does too and which get documents; 23 — the UDP-declared half — came back as a member of the TCP registry, so type() and .proto were wrong whatever the service string said; and 2 named a service UDP does not answer for the port at all. Those two are the whole blast radius: port 888, accessbuilder → TCP's cddbp, and port 999, puprouter → TCP's garcon against UDP's applix. #759's body found the first and generalised from it. After the fix: 10,625 refusals, 0 answers.

Population, since four figures here are easy to confuse. 12,391 declared members (TCP 6147, UDP 6143, SCTP 91, DCCP 10); of those 10,625 carry a multi-bit .proto and 1,766 a single-bit one; they sit on 12,341 distinct (registry, port) pairs (TCP 6121, UDP 6119, SCTP 91, DCCP 10), the other 50 sharing a port with another service as IANA's three on TCP/80 do. The invariance digest covers those 12,341 rows — one per (registry, port), as registry|port|resolved service|int(resolved proto), each looked up by passing the registry's own single transport bit whatever the member's .proto holds: 3a457683c7b00609b06526aa02e3c361a910fa4a0e22d25ca16b4ab01acc053a, identical either side of the fix and identical again with proto passed as a name. So it is the argument that is single-bit, not the member set — an earlier revision of this description labelled it "12,341 single-bit ports", which is a set that does not exist. Restricted to the 1,766 single-bit-.proto members it is 1,763 rows (three share a port), digest 308715ae6642a91be547d73e1c07f84bed7f27b7158951e831201c936807814b, also identical either side. All four counts are asserted in the test.

Regeneration. The change is in the crawler's BASE template; the const file is regenerated from it against IANA's live CSV (sha256 34328ed0…, re-fetched and unchanged), which reproduces all five committed files byte-identically before the change, so the diff is only the guard and its comment. Members unchanged. Siblings byte-identical, md5 before == after: be2b6a48 tcp.py, 6e169b8a udp.py, 39a7749d sctp.py, 556f8403 dccp.py.

Tests. tests/const/test_const_apptype_split_unit.py 15 → 19 tests, 808 → 892 subtests, agreeing under python -m unittest (19 OK). The two refusal tests fail against the pre-fix const file under both runners; the two invariance tests (single-bit resolution, and an AST pin on all ten library call sites passing one transport protocol) pass either side by design. Coverage of the changed file 51.752% → 51.843%: +4 statements, +2 branch arcs, 0 new misses. pylint reports the same 14 findings as main for these two files, mypy and isort clean. make test not run in full — targeted selections only (tests/const, tests/vendor, tests/project = 251 tests / 2382 subtests, plus the transport/schema/corekit/foundation callers = 111 / 188).

Label-worthy: this regenerates a const table, and it is a public-contract change — AppType.get(port, proto=<composite>) previously returned a member and now raises. No in-library caller passes a composite; register_apptype reads a member's composite proto but tests it with in and never reaches the lookup.

[JarryShaw]

Cross-review at 8261139fb (sonnet, a different model from the author): the code fix is verified correct and safe, but three published claims are wrong, so this goes to review: needs-changes. None of them touches the fix itself.

1. The "12,341 declared single-bit ports" figure is wrong, and I re-derived it myself rather than take the reviewer's word. Measured on fe80b8525 with tree provenance printed:

total member rows : 12391    (TCP 6147 UDP 6143 SCTP 91 DCCP 10)
  multi-bit proto : 10625
  single-bit proto: 1766
distinct (reg, port, svc) single-bit: 1766

So the single-bit set is 1,766, not 12,341 — and 12,341 is 50 short of the total 12,391, not of the single-bit subset, which suggests the figure was a total with something excluded and then mislabelled. That makes digest 3a457683c7b00609b06526aa02e3c361a910fa4a0e22d25ca16b4ab01acc053a unverifiable as described: it may well be a sound invariance check, but not over the set the body names. Fix the count and say plainly what the digest covers.

2. tests/const/test_const_apptype_split_unit.py:406 cites a class that does not exist. ConstBuiltinParityTests is nowhere in the tree; the real class is ConstEnumBuiltinParityTests at tests/const/test_const_enum_builtin_parity.py:186. Comment-only, but a wrong cross-reference in the one docstring that explains why ProtocolError is safe here and not in _missing_.

3. The changelog checkbox is unticked with no annotation. House form is N/A — changelog centralised in #657.

One overstatement worth correcting while you are in there. The body says only the UDP half of the 46 is #759's cross-registry dispatch. Measured: of the 23 UDP-origin mismatches, only 2 (ports 888 and 999) actually return a different final service than UDP's own single-bit lookup would; the other 21 land on TCP mechanically but coincide with UDP's canonical string, exactly like the TCP-origin cases. The 23/23 split is right, the characterisation of what each half means is not.

Verified good, independently derived. The ten call sites are confirmed by direct read before the reviewer consulted your AST test — transport.py:187 fed by six literal _make_port(…, Enum_TransportProtocol.<x>) calls, plus three per schema file (_dispatch + two get), each bound to a literal member; your AST test independently pins the same 10 lookups + 6 feeders. register_apptype (protocols.py:831-846) reads code.proto, tests with in, fans into both registries, and never reaches _dispatch. ProtocolError(BaseError, ValueError) propagates untouched: all three proto-bearing calls sit outside the local try/except ValueError, which wraps only owner._missing_ and re-raises any BaseError regardless. The sys.tracebacklimit mechanism is real at exceptions.py:191-200 and your save/restore matches QuietExceptionTests.setUp/tearDown's idiom exactly. The 46 / 0 exceptions / 20 ports / 23-23 split all reproduced. Both failing tests confirmed failing against the pre-fix const file — ProtocolError not raised, and 10,625 unexpected answers — and the two invariance pins judged to earn their place rather than pad. The pre-existing PORT_9899_sctp leak at :213 is confirmed present identically on fe80b8525, and your svc == 'unknown' filter is sound rather than papering over it: it excludes any mint by symptom, order-independently. breaking agreed, with no other undisclosed contract change found. CI 27 success / 3 in progress / 0 failures.

Not verified by the reviewer: the live-IANA byte-identical regeneration, and the coverage/pylint deltas — out of its budget, not disputed.

[JarryShaw]

All three corrected at 515a82633, and the reconciliation is right — 12,341 was a real number that I said was wrong. Verified independently from the committed const files:

tcp   6121 distinct ports of 6147 members
udp   6119 distinct ports of 6143
sctp    91 of 91
dccp    10 of 10
        ----
      12341 distinct (registry, port)   vs 12391 members   difference exactly 50

So 12,341 is the distinct (registry, port) count, and 50 members share a port with another. My correction was about the label — "declared single-bit ports" — not the value: the single-bit-.proto population is a different set at 1,766, which is what I measured. Both figures are sound; only the caption was wrong. Good call adding the second digest (308715ae…, 1,763 rows over the 1,766 members, three sharing a port) so the two sets are distinguishable, and better still asserting all four counts in the test instead of narrating them — the relationship is now enforced in CI rather than trusted.

The blast radius is now stated honestly, and pinned. Of the 46: 44 are get's documented behaviour (the member is simply not its port's canonical); 23 returned a member of the wrong registry; and 2 returned a service UDP's own lookup does not give — UDP/888 accessbuilder → TCP's cddbp, and UDP/999 puprouter → TCP's garcon against UDP's applix. Re-derived two independent ways and agreeing, with assertEqual(divergent, [('UDP', 888, …), ('UDP', 999, …)]) holding it. "2 wrong answers in 10,625 lookups, 23 wrong objects" is the sentence a reader should leave with, and #759's body generalising from 888 alone is what produced the looser framing.

ConstEnumBuiltinParityTests.test_the_exception_is_not_an_in_library_one is the right citation and naming the method is better than the class. Changelog annotated.

Two things done right that I did not ask for. You re-fetched IANA before regenerating, confirmed the CSV byte-identical to your baseline (34328ed0…, 1,157,052 bytes), and kept the siblings byte-identical — so the NOTE comment landing in the crawler template did not smuggle in drift. And you caught that main moved to 0419c1c97 under you, recognised that conflict-free is not safe when #782 touches the file that renders your template and the guard that audits your test file, and ran the merge: 124 passed / 1892 subtests plus 100 / 524 on the tier guard, no interaction.

Label back to review: pending for the new head; re-review dispatching shortly — holding it one tick on host load rather than firing into a box at 3.6.

[JarryShaw]

Round-two cross-review at 515a82633 (sonnet, a different model from the author): GOOD TO GO.

All three required changes confirmed applied, and the reviewer independently recomputed the four counts rather than accepting either my figures or the author's: 12,391 members / 10,625 multi-bit / 1,766 single-bit / 12,341 distinct (registry, port) — 6,121 TCP + 6,119 UDP + 91 + 10 — difference exactly 50. That is a third derivation agreeing with mine and the test's.

The const-file delta really is comment-only. Diffing 8261139fb..515a82633 on both apptype files, every changed line is a # comment; filter comments out and nothing remains. So regenerating from the template to carry the NOTE could not have moved a member, and the unchanged 51.843% coverage follows. Sibling md5s identical across both rounds.

The divergent pin has teeth. divergent is built from live TCP.get(port).svc != cls.get(port).svc comparisons over all 46 (:598, :684) and asserted at :625 — not hardcoded — so a third diverging port would genuinely fail. And I confirmed the four counts are asserted, not narrated: :677 12,391, :680 10,625, :681 1,766, :697 12,341, plus :578 sweeping 10,625.

One honest limit worth recording. The reviewer could not reproduce the exact digest hex 3a457683… or 308715ae… — it got the right row counts (12,341 and 1,763) but a different hash, not knowing your row serialisation. Following that up: grep -c "hashlib\|sha256" on the test file is 0, so neither digest is computed or asserted anywhere — they are prose in a docstring. That is fine, because the count and divergent assertions are the real regression protection, but a reader should not mistake those hex strings for something CI enforces. If you want them enforced, that is a separate small change; I am not asking for it here.

The merge check was redone rather than trusted. Merged 515a82633 into current 0419c1c97 in a scratch worktree — clean, same three-file / 554-line diff — and ran with pcapkit.__file__ printed to prove the tree: tests/vendor + tests/const 124 passed / 1892 subtests, tests/test_tier_guard.py 100 passed / 524 subtests. Both match your figures exactly. That mattered because #782 touches the file that renders your template and the guard that audits your test file.

Settled in round one and not re-run: the ten call sites, register_apptype never reaching _dispatch, ProtocolError propagation, the tracebacklimit save/restore, the two pre-fix failures, the PORT_9899_sctp leak, and breaking.

Six CI legs still outstanding (21 success, 3 expected skips, 0 failures), so this is a review verdict rather than a merge signal. Host load ran 3.08 → 4.84 during the review from other agents, so it was deliberately kept to the requested checks.

[JarryShaw]

why not f-string?

[JarryShaw]

Because this region is a str.format() template in the vendor generator, not hand-written source. pcapkit/vendor/reg/apptype/apptype.py emits the const file through .format() — you can see the delimiters in the same template at :243, __registries__: 'dict[TransportProtocol, Type[{NAME}]]' = {{}}, where the empty dict has to be written {{}} to survive expansion.

So emitting an f-string means writing f'{{proto!r}} names {{len(namespaces)}} …' in the template: every brace doubled, and the generator line stops reading like the code it produces.

Measured, so the local-versus-global convention is explicit:

scope	raise …Error(f'…	raise …Error('…' % …)
pcapkit/ whole tree	605	2
const/reg/apptype/apptype.py on origin/main	0	6

So the repo convention really is f-strings, and this generated file is the exception — all six of its pre-existing raises are %-style (:92, :2308, :2368, :2378, :2478, :2484), and the new one matched them rather than being the only f-string in the file.

Happy to switch it if you would rather the emitted code follow the tree convention — it is one raise, so four doubled braces in the template. Say which and I will make it match.

[JarryShaw]

yea, id like to keep f-string convention across the library. only use % substitution when inevitable.

[JarryShaw]

Recorded, in your words: "yea, id like to keep f-string convention across the library. only use % substitution when inevitable."

It is not inevitable here — the doubled-brace form survives the generator's .format() intact. Verified before routing it, template in and expanded code out:

raise ProtocolError(f'{{proto!r}} names {{len(namespaces)}} transport protocols, and so '   <- in the template
                    f'{{len(namespaces)}} registries of {{cls.__name__}}; look the port up '
                    'under one transport protocol at a time')

raise ProtocolError(f'{proto!r} names {len(namespaces)} transport protocols, and so '        <- emitted
                    f'{len(namespaces)} registries of {cls.__name__}; look the port up '
                    'under one transport protocol at a time')
expanded raise parses as valid Python: True

So the new raise converts, alongside the loop flattening, in the same amend.

Scoping the other six deliberately, rather than quietly. This file has 6 pre-existing %-style raises — :92, :2308, :2368, :2378, :2478, :2484 — all of which your convention also wants converted. I am leaving them out of #783 and filing them as their own issue, because #783 is the multi-bit-proto refusal and a const diff that also sweeps style across a 12,391-member generated file is much harder to review for the thing it is actually doing.

Say the word if you would rather have all seven in one pass here and I will fold them in instead.

[JarryShaw]

Correcting myself on one detail above. I said this region is a str.format() template. It is not — BASE at pcapkit/vendor/reg/apptype/apptype.py:93 is a lambda returning an f-string:

BASE = lambda NAME, DOCS, FLAG, TABLE, MISS, MODL: f'''\

The brace-doubling conclusion is unchanged, since an f-string escapes braces the same way — and :243's = {{}} is still the evidence for it. But anyone following my earlier wording would go looking for a .format() call that is not there. The generated raise and the flattening are unaffected.

[JarryShaw]

if already raised on multiple namespaces, then why the loop on one element list?

[JarryShaw]

You are right, and it is a real simplification — the loop cannot iterate more than once.

After the len(namespaces) > 1 raise directly above it, namespaces is length 0 or 1, so the for is an if wearing a for. Both surviving lengths measured:

undefined  int=0   show_flag_values=[]     len=0
tcp        int=1   show_flag_values=[1]    len=1
tcp|udp    int=3   show_flag_values=[1, 2] len=2   <- raises above

The 0-length case is live, not theoretical — it is the default argument reaching the delegating path:

AppType.get(80, proto=undefined)
  -> ValueError: <TransportProtocol.undefined: 0> names no transport protocol registry of AppType

so the fall-through to the trailing ValueError has to stay. What the loop was doing was making a one-or-zero case look like an n-case, which reads as if a composite could still be resolved here — the opposite of what the raise above it decided.

Flattening to:

if namespaces:
    subclass = cls.__registries__.get(TransportProtocol(namespaces[0]))
    if subclass is not None:
        return subclass
raise ValueError(...)

Applying that to the generator template and regenerating, then re-posting the revision. Flipping to review: needs-changes until it lands.

[JarryShaw]

Delta cross-review at 9aada6af5 (sonnet, a different model from the author): GOOD TO GO — with one refutation that was aimed at my own brief, not at the code.

What it re-derived independently, not accepted:

• The flattening is exactly equivalent. It built both the old for-tail and the new if-tail as standalone functions and swept every TransportProtocol value 0–15 — all 16 combinations of the four declared bits — plus undeclared bits 16/17/31/32. Every result byte-identical. -1 raises inside show_flag_values itself, before reaching this code, identically either way.
• The f-string emits a byte-identical message, checked for 2, 3 and 4 namespaces, and against the live exception from get(888, proto=tcp|udp).
• The hand-edited const file matches what BASE now emits — 78 lines each, 0 diff lines, independently reproducing my own count.
• pylint reproduced exactly: 9.90 → 9.89, 6 → 7 R0801 hunks, and it located the split at line 2413 — the one line where the template has {{proto!r}} and the output has {proto!r}. Diffing the two logs shows no other finding changed. mypy: both files clean (the 111 errors are pre-existing, in unrelated import-followed modules).

The refutation, and it was right. My brief said the branch sits on top of current origin/main. It does not — its parent is fe80b8525, four commits behind ef859f776. I confirmed that, and confirmed none of the four touch these files, so I did not rebase (that would void the verdict for nothing). Instead I merged into current main in a scratch worktree and ran the tests on the merge result — because green and conflict-free is not the same as safe:

merged tree f38403b9c  (carries both this fix and #784's escape_key)
tests.const.test_const_apptype_split_unit          Ran 19 tests  OK
tests.vendor.test_vendor_reg_apptype_generator_unit Ran  6 tests  OK
tests/project (isort gate + dependency gates)      Ran 127 tests  OK

One thing it flagged worth keeping: the # NOTE: block above _dispatch is not stale — it describes the old LSB-first behaviour in past tense as justification for the refusal, not as current behaviour. And consider-using-f-string is still earned, for the six remaining % raises I am filing separately.

Setting review: good-to-go. Unpublished and unmerged — yours to take.