fix(foundation): let register_extractor_* accept pcapkit's own built-in classes

[JarryShaw]

Closes #513.

The defect

The three register_extractor_* helpers validated with issubclass against the public Engine, Reassembly and TraceFlow classes. But every built-in declines auto-registration by subclassing the corresponding *Base class under an alias — from ...engine import EngineBase as Engine and siblings — so none of them is a subclass of the public class at all.

Measured descendant counts on main:

public class	descendants	base class	descendants
Engine	0	EngineBase	9
Reassembly	0	ReassemblyBase	5
TraceFlow	0	TraceFlowBase	2

So register_extractor_engine(PCAP), register_extractor_reassembly(IPv4) and register_extractor_traceflow(TCP) all raised RegistryError for the library's own types — the one set of arguments a user is most likely to pass.

The fix

Widen the three checks at pcapkit/foundation/extraction.py:395, :419 and :443 to the *Base classes, each with a # NOTE: explaining why. Strictly a widening: each public class subclasses its own base, so nothing previously accepted is now refused.

It also brings the two registration paths into agreement. Reassembly.__init_subclass__ calls Extractor.register_reassembly(protocol.lower(), cls) with no issubclass gate at all, so self-registration already accepted anything deriving from the public alias — the explicit helpers were the stricter of the two paths for the same registry.

Tests

tests/foundation/registry/test_foundation.py gains test_registration_accepts_pcapkit_own_builtin_classes: register each built-in through the public helper, assert the registry actually holds it afterwards, and assert a non-subclass still raises RegistryError. Deliberately unmocked, so it exercises the real check rather than a stand-in.

Asserting "no exception raised" is not enough — it passes against a helper that validates and then silently drops the class — so the test reads each registry back, asserting both membership and identity. That read-back was missing from the first revision of this PR and its description wrongly claimed otherwise; a cross-review demonstrated the gap by inserting a bare return after the gate and before the registry write, against which the test still reported 1 passed, 6 subtests passed, exit 0. Added in 733000f65.

Proven to fail without the fix

Reverting only the three issubclass targets and re-running:

	result
with the fix	3 passed, 10 subtests passed, exit 0
three checks reverted to the public classes	3 failed, 3 passed, 7 subtests, exit 1 — SUBFAILED on engine, reassembly and traceflow

The exit code is the proof, not the summary line. A failing subtest still reports its parent test as PASSED on its own -v line, with the real signal only in the tail summary and the exit code — so reading the per-test line would call unfixed code green. (This is pytest 9.1.1's native behaviour; an earlier revision of this description attributed it to the pytest-subtests plugin, which is not installed here. The property is therefore stronger than stated, since it cannot be changed by adjusting the dependency set.)

Checked and deliberately not changed

register_dumper at extraction.py:368 has the same shape — pcapkit/dumpkit/common.py defines DumperBase and then Dumper(DumperBase), measured descendants are 0 and 3, and both dumpkit/null.py:18 and dumpkit/pcap.py:16 import DumperBase as Dumper. But it is not the same defect: extraction.py:23 imports Dumper from dictdumper.dumper — the third-party ABC, not pcapkit's public class — and DumperBase subclasses it, so issubclass is already True for PCAPIO, NotImplementedIO and DumperBase alike. Verified by measurement; left alone.

[JarryShaw]

Cross-review returned NEEDS CHANGES — addressed in 733000f65

The review was right, and the finding was in a claim this PR's own description made. It said the test "reads the registry back rather than merely asserting no exception". It did not: a grep of the committed test for __engine__, __reassembly__ and __traceflow__ returned nothing.

The reviewer did not stop at pointing that out — it built the counterexample, inserting a bare return immediately after the issubclass gate and before the registry write, leaving validation intact while dropping the class. The test still reported 1 passed, 6 subtests passed, exit 0. So it would have blessed precisely the implementation the description claimed it excluded.

Fixed, with the proof

The accept loop now carries each registry and asserts both membership and identity after every call.

state	result
read-back present, fix intact	3 passed, 10 subtests, exit 0
register_engine mutated to validate then drop	SUBFAILED(kind='engine', accepted=True), exit 1

That mutation is exactly what the previous test passed clean. isort -l100 -ppcapkit --check-only clean.

Two corrections to this description, now applied

• The read-back claim above — it is true as of 733000f65 and was false before.
• pytest-subtests is not installed; pytest is 9.1.1, which has subtests natively. The reasoning that the exit code is the proof still holds — the reviewer confirmed the parent test reads PASSED on its own -v line with the real signal only in the summary and exit code — and is in fact better founded than stated, since it is pytest's own behaviour rather than an optional plugin's and cannot be avoided by changing dependencies.

What the review confirmed independently, more rigorously than the description had

Descendant counts exact, by importing every submodule with zero import errors and walking subclasses transitively: Engine 0 / EngineBase 9 — DPKT, Engine, PCAP, PCAPNG, PCAP_CT, PyPCAP, PyPCAPFile, PyShark, Scapy — Reassembly 0 / ReassemblyBase 5, TraceFlow 0 / TraceFlowBase 2.

"Strictly a widening" confirmed beyond the zero-descendant coincidence. It verified direct inheritance in the class statements, then checked for ABC virtual-subclass registration via .register( and for __subclasshook__/__subclasscheck__ overrides on all three metaclasses. None exist — they only add name, module and protocol properties — so issubclass transitivity is ordinary Python semantics and no counterexample is constructible. That closes the one route by which a widening could secretly narrow something.

register_dumper correctly out of scope, matching the 0-and-3 descendant split described here.

And the secondary claim corroborated: Reassembly.__init_subclass__ at pcapkit/foundation/reassembly/reassembly.py:499-520 unconditionally calls Extractor.register_reassembly(protocol.lower(), cls) with no gate of its own, so any class reaching that hook is structurally already a ReassemblyBase — the explicit helper genuinely was the stricter of the two paths.

On #514

Assessed as a reasonable interim step rather than an entrenchment: this touches only the three validation gates, leaving the fallback-registration mechanism, the aliasing idiom and the registries alone — and #514's own proposal keeps *Base as a backward-compatible alias, under which issubclass(x, EngineBase) stays correct. So #523 should not need revisiting when #514 lands.

[JarryShaw]

Re-review of 733000f65: GOOD TO GO

Second cross-review, different model, scoped to the delta 532b9aa93..733000f65 — one file, 24 insertions, 7 deletions.

The hole is closed, and all three gates are independently exercised

state	result
fix intact	3 passed, 10 subtests, exit 0
register_engine validates then drops	SUBFAILED(kind='engine', accepted=True), exit 1
register_reassembly validates then drops	caught — 'unit-513-reassembly' not found in {...}, exit 1
register_traceflow validates then drops	caught — 'unit-513-traceflow' not found in {...}, exit 1

That last pair matters and I had not checked it: mutating each gate individually confirms none is a free rider on another's assertion. Each mutation was applied, tested and reverted with git status --porcelain verified clean between runs.

Four wrong implementations tried, all caught

It was asked to hunt for an implementation the amended test still blesses. It tried:

1. Wrong key — register_engine writes name + '-mutated' → caught, 'unit-513-engine' not found in {...}
2. Truthy but wrong value — cls.__engine__[name] = True → caught by the identity assertion specifically, True is not <class '...PCAP'>
3. Wrong registry — register_engine writes into cls.__reassembly__ → caught, key absent from the correct per-kind store
4. ModuleDescriptor instead of the resolved class → caught, ModuleDescriptor(module='pcapkit.foundation.engines.pcap', name='PCAP') is not <class '...PCAP'>

None passed. Membership alone would have missed cases 2 and 4; identity alone would have missed 1 and 3. Both assertions are load-bearing.

The description's corrected claims verified

pip show pytest-subtests → not found; pip list shows only pytest 9.1.1. And the behaviour the description now rests on, confirmed directly in a -v mutation run: the progress line printed test_registration_accepts_pcapkit_own_builtin_classes PASSED [100%] while the real failure appeared only in the FAILURES section, the SUBFAILED short-summary line, and the exit code. So reading the per-test line would call unfixed code green — which is why the exit code is the stated proof.

Also confirmed: the module has no module-level registry or Extractor import — only __future__, importlib.util, unittest, unittest.mock and purge_modules — so importing Extractor inside the test is the correct pattern and matches every sibling, required because setUp calls purge_modules(['pcapkit']). isort -l100 -ppcapkit --check-only exits 0.

Disputes: none

Every claim reproduced with its own numbers.

Out of scope, stated plainly

It did not re-run the descendant-count enumeration or the ABC-registration and __subclasshook__ sweep from the first review — those cover unchanged surrounding context rather than this delta, and nothing in the delta depends on them. Those remain verified by the first cross-review only.

[JarryShaw]

✅ GOOD TO MERGE — cross-reviewed twice; four wrong implementations tried against the new registry read-back and all four caught.