fix(foundation): registrar error messages and type hints name the public class, not the Base the guard checks

Extractor's three registrar methods check against the *Base classes but name the public class in their error message, so a caller who trips the guard is told the wrong type is required.

In pcapkit/foundation/extraction.py:

check	message
`:452` `issubclass(engine, EngineBase)`	`:453` "engine must be an Engine subclass"
`:491` `issubclass(reassembly, ReassemblyBase)`	`:492` "reassembly must be a Reassembly subclass"
`:530` `issubclass(traceflow, TraceFlowBase)`	`:531` "traceflow must be a TraceFlow subclass"

The accepted type is wider than the message says: Engine, Reassembly and TraceFlow are each a strict subclass of their base, and the shipped classes derive from the base directly. So a class that subclasses EngineBase is accepted while the message implies it should not be, and a reader who hits the error is sent to fix something that was never wrong.

Two related narrowings in the same area:

register_extractor_engine and its siblings are annotated Type[Engine], narrower than the runtime check, so a legitimate EngineBase subclass is a type error at a call site that works at runtime.
register_protocol checks issubclass(protocol, ProtocolBase); its message and annotation should be checked for the same mismatch.

Found while sweeping docstrings for #719. The prose side is corrected in #1015, which deliberately left these alone because the messages and annotations are code, not documentation. #513 and #514 are the same conflation approached from the caller's side.

The fix is a judgement call rather than mechanical: either widen the messages and annotations to the *Base classes, matching the guard, or narrow the guards to the public classes, which would be a behaviour change and needs a ruling.

wait.. we had a design decision that all internal in-house subclasses must inherit directly from the XXXBase classes and any 3P customised extended subclasses must inherit from the XXX classes instead - correct me if we decided otherwise already.

You are right, and I cannot correct you — the code says exactly that, in all three places. engines/engine.py:88, reassembly/reassembly.py:123 and traceflow/traceflow.py:126 each carry the same note: the *Base class is for internal use only, and for customisation use Engine / Reassembly / TraceFlow instead. The public classes' docstrings are correspondingly the extension surface — each opens with registration being opt-in via a keyword at class definition. And every shipped in-house class does inherit the base directly, which is consistent.

That makes #1020 wrong-headed as it stands, and I would rather say so now than have it merge. Its messages now read "must be an EngineBase subclass". An error message is user-facing: the person who sees it is whoever just passed a class that was rejected, and for a third-party extender that is a pointer at a class whose own docstring tells them not to use it. Technically accurate about the guard, actively misleading as guidance — which is a worse defect than the one #1016 reported.

Reframing #1016, since the original complaint does not survive contact with the design. The message was not naming the wrong type by mistake; it was naming the type a third-party caller is supposed to use. What is genuinely wrong is narrower:

The annotations were too narrow — Type[Engine] where the guard accepts EngineBase, so mypy rejects storing an in-house base-only class that the runtime accepts. That half of fix(foundation): name the base class the registrar guard actually checks #1020 is right and should stay.
The Raises: prose is where the precise accepted type belongs, and docs(foundation): correct the registrar contracts and cut timed context #1015 already corrected it to say the base is accepted. Prose carries precision, message carries guidance.

My proposal: revert the four message strings to naming the public class, keep the annotation widening, the unused-import removal and the cast, and adjust the test to assert the public-class name. The test still earns its place — it pins that a class subclassing only the base is accepted, which is the in-house path and was previously undocumented and untested.

One decision I would rather not guess at, so I am labelling this needs: decision: should the message name only the public class (cleanest, matches the docstring guidance), or name it with a parenthetical for the base, e.g. "must be an Engine subclass (or EngineBase for in-house use)"? The second is more honest about the guard but leaks the internal class into a user-facing string.

Holding #1020 until you say. Its cross-review is running on a brief that assumed the base-naming was correct, so I have told it to treat that premise as now doubtful.

Worth noting for the record: this rule exists only in those three class docstrings, nowhere in docs/source/contributing/. Given we are already writing a placement-rule page for #719, this belongs written down too — it is exactly the kind of decision that a future sweep would otherwise "correct" away.

the message names only the public class

and since in-house classes are manually registered by mapping already, we dont have to keep the door open for them to be registered through the register_xxx APIs again.

Both rulings taken, and your premise checks out — more cleanly than I expected. Every in-house entry in Extractor.__engine__, __reassembly__ and __traceflow__ is a ModuleDescriptor, not a class, and the guard tests isinstance(x, ModuleDescriptor) before it reaches issubclass. So in-house registrations never touch the guard at all. The only register_* call sites in the package are docstring examples, the public classes' own __init_subclass__ (which passes a public-class subclass by construction), and the wrappers.

So narrowing is safe for the three foundation registrars, and it makes the message correct by construction rather than by convention. Plan for #1020: guards become issubclass(x, Engine / Reassembly / TraceFlow), messages name those same public classes, annotations stay as they are (Type[Engine] was already right — the widening I had added is exactly what should be dropped), and the test flips to pinning that a base-only class is now rejected.

That is a behaviour change — a third party subclassing EngineBase directly and calling register_engine is rejected where today it is accepted. That is the door you say we need not keep open, but it earns a breaking label and a 1.5.0 changelog entry rather than passing as a fix.

One place your ruling cannot apply, and it is #514's subject. register_protocol guards ProtocolBase, and:

Protocol descendants     : 0
ProtocolBase descendants : 44

The public Protocol class has no descendants at all — every in-house protocol subclasses ProtocolBase directly, and unlike the engine case those are real classes reached through the protocol registries. Narrowing register_protocol to Protocol would reject all 44. And leaving the guard at ProtocolBase while naming Protocol in the message would reintroduce exactly the mismatch #1016 opened on.

So I am leaving register_protocol entirely untouched — guard and message both as they are today — and proceeding on the three foundation registrars, which are unblocked. The protocol side is the same conflation #514 is about: the Protocol/ProtocolBase split does not currently behave like the Engine/EngineBase one, because nothing inherits the public class. Keeping needs: decision on for that, since it wants your call and not my guess.

Worth repeating one thing from earlier: the in-house-versus-third-party rule lives only in three class docstrings and nowhere in docs/source/contributing/. Given it has now twice changed what a PR should do, it should be written down alongside the #719 placement rule.

Correcting myself twice, and both corrections strengthen your ruling.

I said this rule lives only in three class docstrings and nowhere in the docs. Wrong on both halves. It is in five *Base docstrings — add protocols/protocol.py:285 and dumpkit/common.py:50 — and docs/source/ext.rst states it twice with the reason I was missing: at :383 and :707-708, the public class is the one to build on because it is the one carrying the registration hook. Only docs/source/contributing/ is silent, so the gap I flagged is real but much smaller than I described.

That reason is decisive on its own, independent of the docstrings. The public class is the registration surface, which is exactly why narrowing the guard to it is correct rather than merely tidy.

Independent corroboration I had not spotted: register_dumper already does it. At extraction.py:400 it guards the public Dumper and names Dumper in the message — the fourth registrar never left the pattern. The other three check the base for an internal reason the code states at :447-452: the built-ins derive from the base so they are not auto-registered, which made a public-class check reject pcapkit's own classes. So the wide guard was a tolerance for our own built-ins, not a contract offered to callers.

One count to fix for the record: there are four base-only reassembly classes, not three — reassembly/ip.py:IP plus IPv4, IPv6 and TCP, with IPv4/IPv6 deriving from IP. Three is the __reassembly__ entry count. Totals: 8 engines, 4 reassembly, 1 traceflow — 13 of 13 built-ins on the base, none on the public class.

The cross-review came back NEEDS CHANGES on the current head and reached the same conclusion about the premise independently. Rework is in flight; register_protocol stays untouched for the reason above.

One more measurement that changes what #1020 has to do, and it makes your ruling cheaper to satisfy than I thought.

main already names the public class in every one of these messages. extraction.py:453 reads engine must be an Engine subclass, and registry/protocols.py:217 reads protocol must be a Protocol subclass. So #1020's edit was not fixing a message that named the base — it introduced the base-naming that your ruling rejects. The whole message half of that PR reverts to main rather than needing new wording.

That also settles register_protocol without needing your call. Its message already names Protocol, which is what you ruled for; its guard stays at ProtocolBase, which it must, because 43 classes derive from the base and none from the public one. The gap between guard and message is therefore permanent there — and correct, because the message's reader is a third party who should be told Protocol. The Raises: prose is where the base belongs, and it already says so. I am keeping needs: decision on only for the #514 question of whether that asymmetry should be redesigned, not for the message wording.

Two defects the cross-review turned up on the way, both pre-existing and both filed separately rather than folded in here: the Raises: clauses promise RegistryError for a non-class input and the code raises TypeError; and _exeng / Extractor.engine / record_header are declared Engine while always holding base-only instances, with two casts papering over it.

Stop before this lands — the narrowing reverts #513, and #513's own regression test fails under it. Also correcting a mechanism claim I made two comments up, which was wrong.

What I got wrong. I told you the guard tests isinstance(x, ModuleDescriptor) before issubclass, so in-house registrations never touch it. The isinstance is there, but it unwraps rather than short-circuits (extraction.py:445-446): engine = engine.klass, then the guard runs on the resolved class. So after narrowing, register_engine('x', ModuleDescriptor('pcapkit.foundation.engines.pcap', 'PCAP')) is rejected. My conclusion happened to survive, but for a different reason: the shipped entries are dict literals in the class body and never pass through the registrar at all. Measured entry counts — __engine__ 6, __reassembly__ 3, __traceflow__ 1, all ModuleDescriptor; pcap and pcapng are not in the mapping, they are reached by the default branch in run().

The conflict. tests/foundation/registry/test_foundation.py:130-160 is #513's regression test. It deliberately passes the real base-only built-ins — PCAP, IPv4 reassembly, TCP traceflow — through register_extractor_* unmocked, and reads the registry back, with a comment saying it does not mock because the point is to exercise the gate. Under the narrowed guard it fails 3 of 3:

RegistryError: traceflow must be a TraceFlow subclass, not <class 'pcapkit.foundation.traceflow.tcp.TCP'>

And main's own NOTE at the guard says the base check exists because checking the public class "made this check reject pcapkit's own classes. See #513." So #1016's narrowing re-asserts precisely what #513 called the defect. I left that test untouched rather than gutting it to go green.

Three ways out, and I would take the third:

Narrowing wins, register_extractor_engine/reassembly/traceflow reject pcapkit's own built-in classes #513's test is rewritten to pass a public subclass. Cheapest, but it discards what register_extractor_engine/reassembly/traceflow reject pcapkit's own built-in classes #513 bought — that pcapkit's own classes are registrable through the public API.
register_extractor_engine/reassembly/traceflow reject pcapkit's own built-in classes #513 wins, guards stay wide. fix(foundation): name the base class the registrar guard actually checks #1020 then closes as unnecessary, since main already names the public class in every message.
Narrowing wins and the built-ins get an internal registration path that bypasses the public guard. Keeps your "the public door names the public class" and register_extractor_engine/reassembly/traceflow reject pcapkit's own built-in classes #513's "our own classes are registrable" without either conceding.

Holding #1020 and labelling it blocked.

Anything still needs my decision?

Yes — one, and it is the only thing left on this issue.

Does register_engine/register_reassembly/register_traceflow narrow its guard to the public class, or stay wide? Your ruling that in-house classes need not keep the register_* door open implies narrowing, and I reworked #1020 to do exactly that. It then broke #513's own regression test, which deliberately pushes the real base-only built-ins (PCAP, IPv4 reassembly, TCP traceflow) through register_extractor_* unmocked and reads the registry back — 3 of 3 subtests fail under the narrowing. main's own comment at the guard says the wide check exists because checking the public class "made this check reject pcapkit's own classes. See #513." So the two decisions genuinely collide and I will not pick between them for you.

The three options are in my 01:22Z comment. My recommendation is the third: narrow the public door and give the built-ins an internal registration path that bypasses the guard, so your "the public door names the public class" and #513's "our own classes are registrable" both hold. The first option discards what #513 bought; the second closes #1020 as unnecessary, since main already names the public class in every message.

Nothing else here needs you. The message wording is settled — main was already correct and #1020 had regressed it. register_protocol is settled: its message already names Protocol, its guard must stay at ProtocolBase because 43 classes derive from the base and none from the public one, and the permanent gap between them is correct because the message's reader is a third party. The two defects found along the way are filed separately as #1021 (ruled, option 2) and #1022 (fixed in #1023), and #1024 tracks EngineBase being excluded from __all__ while documented :no-members: — that one touches the same in-house-versus-third-party split, so it is worth settling alongside whichever option you pick here.

Yes take the third and follow your recommendations.

Taken — the third option, so neither your ruling nor #513 concedes.

What lands, concretely. The three foundation registrars narrow to issubclass(x, Engine / Reassembly / TraceFlow), and their messages stay as they are on main, which already name those public classes. The built-ins stop going through the public door: they get an internal registration path that writes the mapping directly, bypassing the guard. #513's regression test then keeps its meaning — pcapkit's own classes are still registrable — but it exercises the internal path instead of the public one, so the test is retargeted rather than weakened. I will make that retargeting explicit in the diff and say in the PR why the assertion moved, since a reader coming from #513 should not have to guess.

register_protocol stays untouched for the reason established earlier: 43 classes derive from ProtocolBase and none from the public Protocol, so narrowing it would reject every in-house protocol class. That asymmetry is #514's subject, not this one.

Dropping needs: decision and unblocking #1020, which carries the rework. Also unblocking #1024 — your choice settles the question it was waiting on: EngineBase stays an internal class for registration purposes, yet #1023 makes it the declared return type of a public property, so the docs fix is to give it documented members rather than to export it in __all__.

fix(foundation): raise RegistryError, not a leaked TypeError, for a n…

fix(foundation): narrow the registrar guards to the public classes

docs(foundation): document the engine members on EngineBase, not Engi…

fix(foundation): name the base class the registrar guard actually che…

docs(foundation): document the engine members on EngineBase, not Engi…

fix(foundation): raise RegistryError, not a leaked TypeError, for a n…

Closing: #1020 landed this, and I verified it on main 51100da7e rather than inferring it from the merge.

#1020 merged as cacbf2e70 but carried no Closes keyword, so this stayed open. The defect it reported — guard checks the *Base, message names the public class — is gone, and it was fixed by the third option ruled here: narrow the guard to the public class and give the built-ins their own internal door, rather than widening the message to the base.

Measured on main, every guard and its message now agree:

line	guard	message
`:412`	`issubclass(dumper, Dumper)`	"dumper must be a Dumper subclass"
`:457`	`issubclass(engine, Engine)`	"engine must be an Engine subclass"
`:526`	`issubclass(reassembly, Reassembly)`	"reassembly must be a Reassembly subclass"
`:595`	`issubclass(traceflow, TraceFlow)`	"traceflow must be a TraceFlow subclass"
`:484`	`issubclass(engine, EngineBase)`	names `Extractor._register_internal_engine` and `EngineBase`
`:553`	`issubclass(reassembly, ReassemblyBase)`	names the method and `ReassemblyBase`
`:622`	`issubclass(traceflow, TraceFlowBase)`	names the method and `TraceFlowBase`

The type hints this issue also named are consistent now: the public registrars are annotated ModuleDescriptor[Engine] | Type[Engine] (:421, :492, :561) and the internal ones ...[EngineBase] (:462, :531, :600), so the annotation, the guard and the message say the same thing at every door.

Nothing is left outstanding here. The related non-class leak — these guards raising a bare TypeError rather than the documented RegistryError — is #1021, covered by #1025 and still open; the same leak in the seven protocols/ register classmethods is #1026.

fix(registry): foundation registrars restore the built-in a name ship…

docs(protocols): second pass over application, misc and the protocol …

fix(protocols): export Protocol from pcapkit.protocols.protocol (#1770)…

docs(changelog): second whole-repo pass over the changelog (#1103) (#…

Navigation Menu

Repository navigation

fix(foundation): registrar error messages and type hints name the public class, not the Base the guard checks #1016

2 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

fix(foundation): registrar error messages and type hints name the public class, not the Base the guard checks #1016

Description

Activity

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

JarryShaw commented on Oct 5, 2026

2 remaining items

JarryShaw commented on Oct 5, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions