fix(protocols): raise RegistryError for a non-class in seven register guards (#1026)

[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• A test case covers the change — tests/protocols/test_register_class_guard_unit.py, new here. make test itself was not run: the full suite reaches ~56 GB on this host and gets OOM-killed, so the selections below were run instead, narrowly and sequentially.
• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md

What is the purpose of your pull request?

• 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

Closes #1026. The seven register classmethods under pcapkit/protocols/ document RegistryError for a bad argument but leaked a bare TypeError for a non-class, because issubclass rejects a non-class itself before any guard of ours runs. This is the protocols/ half of the same defect #1025 fixes in foundation/.

Each site gains an explicit isinstance(..., type) test ahead of its issubclass, placed after the ModuleDescriptor unwrap where one applies. No guard's target class changes, so a wrong class still raises RegistryError exactly as before.

Measured per site, with the raise location read off traceback.extract_tb(e.__traceback__)[-1] rather than off the message — a plain-type metaclass and an ABCMeta-derived one produce the identical issubclass() arg 1 must be a class text from different frames, so the message cannot distinguish them:

entry point	exception	raised at
ProtocolBase.register	RegistryError	protocol.py:769
Frame.register	RegistryError	frame.py:152
PCAPNG.register	RegistryError	pcapng.py:920
SCTP.register	RegistryError	sctp.py:629
Link.register	RegistryError	link.py:144
Internet.register	RegistryError	internet.py:164
TCP.register	RegistryError	transport.py:115
UDP.register	RegistryError	transport.py:115
Transport.register	UnsupportedCall	transport.py:110

Transport is the subtle one and the issue's own premise was wrong about it: Transport.register's UnsupportedCall is gated on cls is Transport, so it fires only for the abstract class. That gate is precisely why the guard below it was reachable — TCP.register and UDP.register get past it, and both leaked. So this is seven leaking sites, not six plus an exempt one.

Not breaking for exception handling. RegistryError subclasses TypeError through BaseError, so except TypeError still catches it. Two side effects do come with it, because BaseError is loud by default: the non-class path now emits one CRITICAL log record and, outside devmode, installs sys.excepthook and threading.excepthook — neither of which the bare abc TypeError did. Measured on both trees. That is already the wrong-class path's behaviour, so this makes the two consistent.

Verified: the new test has 6 methods carrying 72 subtests. Against main's guards 40 subtests fail and 32 pass; with the subtest-free Transport method that is 33 checks passing either way. Those are genuine guards, not filler: a wrong class still raising (8), a descriptor naming a wrong class still falling through to the subclass check (8), a valid class and valid descriptor still registering (16 — the only thing proving the new isinstance test rejects nothing it should accept), and Transport still short-circuiting (1). tests/protocols/test_register_class_guard_unit.py plus protocol-base, registry and code-registration: 49 passed, 83 subtests. Dispatch: 24 passed, 87 subtests. tests/protocols/transport + link: 191 passed, 152 subtests. tests/project: 268 passed, 1 skipped, 864 subtests.

[JarryShaw]

Cross-review verdict on 467eced4d: NEEDS CHANGES (ran on Opus; authored on Sonnet). Both findings were prose, the code was confirmed correct, and both are fixed in 0475f2044 — now pushed.

1. The "not breaking" claim was measurably false, and I verified it myself before accepting it. BaseError is loud by default, so the non-class path now does two things the bare abc TypeError never did. Measured on both trees with DEVMODE=False:

	main guards	this change
raised	TypeError: issubclass() arg 1 must be a class	RegistryError: protocol must be a class, not 1
log records	[]	[('CRITICAL', 'RegistryError: protocol must be a class, not 1')]
sys.excepthook replaced	no	yes
threading.excepthook replaced	no	yes

So "only a caller matching the exact type or the old message text sees a difference" was wrong — a caller watching logs or hooks sees it without touching either. The entry now says so, and says the honest mitigating thing: this is already the wrong-class path's behaviour, so the change makes the two consistent rather than inventing a side effect. The review also checked this was not a formula inherited from elsewhere — the sentence is introduced here, with no precedent on main to defer to.

2. "6 pass either way" was wrong; it is 33. I conflated the method count with the pass-either-way count. Measured with a TextTestResult subclass counting addSubTest outcomes:

	main guards	0475f2044
methods	6	6
subtests passing	32	72
subtests failing	40	0

32 passing subtests plus the subtest-free test_transport_itself_is_still_unsupported is 33 checks that hold either way. The 40 is right and splits 32 non-class + 8 descriptor-naming-a-non-class. My gloss also omitted the most valuable guard, test_valid_class_and_descriptor_still_register (16 of the 33) — the only thing proving the new isinstance test rejects nothing it should accept. Both the entry and the commit message now say 33 and name all four guards.

What it confirmed independently, re-deriving rather than accepting: all nine entry points at the exact lines claimed, for all four non-class arguments, with the raise site read off the traceback — and at base every failure pointing at <frozen abc>:123, which is the leak. The Transport gate at :109/:110 with TCP.register and UDP.register both resolving to Transport.register and both reaching :115. That SCTP is separate because it overrides register at sctp.py:592 and never meets the gate. That the unwrap does not short-circuit — a descriptor naming a wrong class falls through to the issubclass line two lines further on. All seven guards byte-identical to main apart from the two inserted lines. And no missed site: it swept every register and every issubclass in pcapkit/protocols/, finding the other four already paired with an isinstance test.

Three things it raised that I am not acting on, with reasons. mypy.ini sets warn_unreachable = True and the narrowing should make the new test unreachable — it does not fire, confirmed against all seven files, though the run surfaces ~24 pre-existing errors elsewhere, so make mypy is not a clean baseline independent of this change. Transport.__proto__ is ProtocolBase's table rather than its own ('__proto__' not in vars(Transport)) — pre-existing and harmless here, but a future mock.patch.dict(Transport.__proto__) would silently patch ProtocolBase's registry. And tests/protocols/transport, link and the full tests/project were left UNVERIFIED on its side; my own runs cover them, and tests/project + the new test on the amended head gives 274 passed, 1 skipped, 936 subtests.

Resetting review: since the head moved.

[JarryShaw]

Cross-review verdict on 0475f2044: GOOD TO GO (ran on Opus; authored on Sonnet). Both earlier findings are fixed, and the review measured the one element of my replacement prose that it had previously only inferred.

The scope claim held, which is what let claims 1–4 and 6 carry across without redoing them: git diff 467eced4d 0475f2044 --name-only touches zero files under pcapkit/ or tests/ — only the changelog pair.

The part worth recording. My new wording says the side effects are "the wrong-class path's existing behaviour, so the fix makes the two consistent". That was my inference. The review measured it on a tree with main's guards restored, passing dict so the wrong-class branch is the only reachable RegistryError: one CRITICAL record, both sys.excepthook and threading.excepthook replaced — on main. So the consistency argument is measured rather than plausible.

It also checked the placement of the devmode qualifier, which I had not: BaseError.__init__ logs CRITICAL on both branches but calls _install_excepthook() only in the else, so attaching "outside devmode" to the install clause alone is exactly right rather than loosely applied to both.

On editing two files a test compares, which was the real risk of hand-amending the changelog pair: extracting the entry from each and normalising markup gives 1521 characters on both sides, identical, and test_changelog_md.py + test_conventions_doc_claims.py give 97 passed, 1 skipped, 184 subtests. The pin is untouched and still correct — the edit reworded inside an existing bullet without adding or removing one, so the count stays 159.

One correction to something I wrote, not to the change: I said the entry and the commit message both now state 33. The entry carries no subtest count at all, which is right — subtest arithmetic belongs in the commit message and PR body, not in a user-facing changelog. Flagging it so nobody looks for a "33" in the entry and concludes the edit was dropped.

The review agreed with all three of my non-actions and is not re-raising them: the warn_unreachable non-finding (with the disposition that make mypy is not a clean baseline, so a ticked style box must not be read as a clean run on any future PR either), Transport.__proto__ sharing ProtocolBase's table, and its UNVERIFIED test set — the last being moot, since this sha changes no code at all.