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

[JarryShaw]

Please follow the guide below

• 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

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

Implements the ruling on #1016. This description replaces two superseded ones — the original widened the guards to the *Base classes, the second narrowed them without an escape hatch, and both were wrong for reasons recorded in the thread.

What the ruling settles. The public door names the public class, and pcapkit's own base-only built-ins get an internal path — so neither that ruling nor #513 concedes. Without the second half, narrowing alone fails #513's own regression test, which deliberately pushes the real built-ins through register_extractor_* unmocked.

The change:

• register_engine, register_reassembly and register_traceflow guard issubclass(x, Engine / Reassembly / TraceFlow) instead of the *Base classes. Their messages are unchanged — main already named the public class, which is exactly the mismatch fix(foundation): registrar error messages and type hints name the public class, not the Base the guard checks #1016 reported, and an earlier revision of this PR wrongly "fixed" them to name the base.
• Three private classmethods take the base check and carry the built-ins: Extractor._register_internal_engine and siblings. They unwrap a ModuleDescriptor, check the base, keep the overwrite warning, and write the registry. None is in any __all__ and no register_extractor_* wrapper reaches them.
• The three registry stores and the three metaclass registry properties widen from Type[Engine] to Type[EngineBase], and three casts are deleted. The internal path writes base-only classes by design, so the narrow declaration became false by construction rather than by accident. dict is invariant in its value type, so the stores could not widen without the properties returning them — engines/engine.py:56, reassembly/reassembly.py:84, traceflow/traceflow.py:85.
• The #: prose above each store claimed the values were "a tuple representing the module name and class name" and an Engine subclass. They are ModuleDescriptors and base-only classes — wrong on both counts before this change.

Untouched, deliberately. register_protocol cannot narrow: ProtocolBase has 43 descendants and the public Protocol has 0, so narrowing would reject every in-house protocol class. That asymmetry is #514's subject. register_dumper already guards the public Dumper — the pattern this PR restores to its three siblings.

Breaking: a third party subclassing EngineBase directly and calling register_engine is now rejected where it was accepted.

#513's regression test is retargeted, not weakened. It keeps its real subject classes, its unmocked gate and its registry read-back, and now exercises the internal path, with a comment citing #1016 so a reader arriving from #513 sees why the assertion moved. What it guaranteed still holds: pcapkit's own base-only classes are registrable. Two new tests pin the other half — the public door rejects a base-only class both as a class and as a ModuleDescriptor — failing 6 of 6 subtests on main.

On the dependency that is now resolved. The store widening draws one mypy error on its own, on self._exeng = eng(self), because _exeng was declared Engine[_P]. That was #1023's declaration, and I deliberately did not silence it with a cast or an ignore in a file another PR owned. #1023 has merged, and rebased onto it this branch measures 305 errors with 0 attributed to extraction.py — identical to main.

tests/foundation + tests/interface + tests/project: 557 passed, 13 skipped, 1300 subtests passed. util/changelog_md.py --check exits 0, and the process.rst entry-count pin is set from measurement.

[JarryShaw]

Unblocked — the maintainer ruled on #1016 and chose the option that resolves the #513 conflict rather than trading one defect for another.

The narrowing goes ahead, and the built-ins get an internal registration path that bypasses the public guard. That is what makes #513's regression test survivable: its point was that pcapkit's own classes must be registrable, and they still are, through the internal path. The test is retargeted at that path rather than deleted or loosened.

So this PR's rework changes shape once more. The guards narrow to Engine/Reassembly/TraceFlow as before, the messages stay as main has them, the annotation widening stays reverted, register_protocol stays untouched — and the new piece is the internal path plus the retargeted test. Removing blocked, keeping review: pending since the head will change.

[JarryShaw]

The ruled design is built and verified, with one ordering constraint: #1023 should merge first.

What is in the rework, off main rather than either of this PR's two superseded heads. The public door narrows to issubclass(x, Engine / Reassembly / TraceFlow) with main's messages unchanged. Three private classmethods — Extractor._register_internal_engine and siblings — take the base check and carry the built-ins, so they never pass through the public guard. The # NOTE: at each guard now explains the narrow check and cites #1016 rather than #513.

#513's regression test is retargeted, not weakened. It keeps its real subject classes, its unmocked gate and its registry read-back, and now calls the internal path, with a comment citing #1016 so a reader arriving from #513 sees why the assertion moved. Two new tests pin the other half: the public door rejects a base-only class both as a class and as a ModuleDescriptor, failing 6 of 6 subtests on main with RegistryError not raised.

One consequence worth stating plainly. The internal path writes base-only classes into stores declared Type[Engine], so those three stores and the three metaclass registry properties returning them are widened to EngineBase and three casts are deleted — dict is invariant, so the stores could not widen alone. That widening draws exactly one new mypy error, on self._exeng = eng(self) at extraction.py:678, because _exeng is still declared Engine[_P] here. That declaration is #1023's, and I deliberately did not silence it with a cast or an ignore in a file another PR owns.

Measured on a scratch tree with #1023 cherry-picked onto main and this rework applied: 305 errors in 33 files, zero attributed to extraction.py, matching main. So the regression is entirely #1023's declaration and clears when it lands. Merge #1023, then rebase this.

Also corrected while in there: the #: prose above each store said the values were "a tuple representing the module name and class name" and an Engine subclass. They are ModuleDescriptors and base-only classes — wrong on both counts before this change, not because of it.

[JarryShaw]

Cross-review verdict: NEEDS CHANGES on prose, code good to go (Opus; author on Sonnet). Three findings, two real and one not — all three re-derived by me.

A. The changelog asserted a mechanism that never runs, and this is the real one. It said the built-ins "are registered through a new internal path". They are not. git grep -n "_register_internal" over the whole head tree returns only the three definitions, the three public wrappers delegating to them, and two test files — zero production call sites. The built-ins are ModuleDescriptor literals in Extractor's class body and never pass through a registrar at all. That is the same class of mechanism error I was corrected on twice in #1016's own thread, and this time it was in a shipped CHANGELOG.md. Reworded to say what actually happens: the built-ins are unaffected because they bypass registrars entirely, and the internal path exists for programmatic internal registration and is what #513's regression test now exercises.

B. "Three casts are deleted" — none were. git diff fa3e861f5...23506be98 | grep -E '^[-+].*cast\(' is empty. That sentence survived from this PR's first shape, which widened the public signatures; once the signatures stayed Type[Engine], the three cast('ModuleDescriptor[Engine]', …) calls in registry/foundation.py became correct and stayed. Sentence deleted.

C. Not a defect — an invocation difference. The reviewer measures 321 errors where the message says 305. Both are right: the message says "mypy on extraction.py", the single-file entry point, and 321 is the whole-package mypy pcapkit form. The message now names both so the figure cannot be read against the wrong invocation.

Head is now dae250597. Everything else reproduced, several points more strongly than the description claimed:

• The register_extractor_engine/reassembly/traceflow reject pcapkit's own built-in classes #513 retargeting holds, and the reviewer judged it independently rather than passing mine through: the test keeps all three real subject classes, stays unmocked, keeps the registry read-back and its measured anti-no-op comment, and gains mock.patch.dict rollback it did not have. It verified the complement is genuinely new by measuring that register_extractor_engine('x', PCAP) is accepted on main.
• Internal-path reachability tested rather than read: every relevant __all__ returns no internal name, getattr(pcapkit, '_register_internal_engine') is missing, and extraction.rst autodocs only the four public registrars — while noting it does autodoc _cleanup, so excluding these was deliberate.
• Counts derived independently: ProtocolBase 44 descendants including Protocol itself, Protocol 0; and the 13 base-only built-ins as EngineBase 8 + ReassemblyBase 4 + TraceFlowBase 1.

One honest gap it flagged: the #513 test no longer exercises register_extractor_* in the accepting direction unmocked — that coverage moved to the new test, which uses locally-defined public subclasses. A real shift, and acceptable, but worth naming.

[JarryShaw]

Verdict carried to dae250597: GOOD TO GO. The cross-review's only objections were to prose, and it said so explicitly — "Code: good to go. Prose: three fixes, no code change needed."

Granting on that basis rather than paying a fourth review, because the amendment is prose-only and I measured it: git diff 23506be98 dae250597 -- pcapkit/ tests/ is empty, the whole diff being CHANGELOG.md and 1.5.0.rst. So every code finding the reviewer confirmed at 23506be98 holds at this head by construction, not by assumption — the same test I applied before carrying #1025's verdict forward.

Both substantive findings are fixed:

• The changelog no longer claims the built-ins are registered through the internal path. It now says they are ModuleDescriptor literals in the class body that never pass through a registrar, and that the internal path exists for programmatic internal registration and is what register_extractor_engine/reassembly/traceflow reject pcapkit's own built-in classes #513's regression test exercises.
• The "three casts are deleted" sentence is gone; none were, and the three in registry/foundation.py are correct now the public signatures stayed Type[Engine].

The third was not a defect: the reviewer measured 321 mypy errors against the message's 305. Both are right for their invocation — 305 is mypy pcapkit/foundation/extraction.py, 321 is whole-package mypy pcapkit. The message now names the invocation so the figure cannot be read against the wrong one.

Re-ran after the amendment: tests/project + tests/foundation/registry → 288 passed, 1 skipped, 968 subtests passed, and util/changelog_md.py --check in step with the pin measured at 157.

Awaiting CI's last legs, then yours to merge. One thing worth carrying into review: the reviewer flagged that #513's test no longer exercises register_extractor_* in the accepting direction unmocked — that coverage moved to the new test, which uses locally-defined public subclasses. Acceptable, but a real shift rather than a pure retargeting.

[JarryShaw]

Withdrawing the good-to-go: this branch roughly doubles the protocols test leg, and I called the failure a flake when it was not.

I reported the first red Plain unittest ordering (protocols) as an infrastructure flake because the log carried The runner has received a shutdown signal, and I discounted the duration. The duration was the real signal. Both attempts died the same way, so I measured the same job across the last twelve runs:

branch	head	result	duration
main	fa3e861f5	success	20.8m
#1025	1f24b9aa9	success	21.5m
main	5fd0a1b11	success	27.0m
this branch	dae250597	failure	42.2m
this branch	23506be98	cancelled	44.9m

Every other run of that job sits at 18–27m. Only this branch reaches the timeout-minutes: 45 neighbourhood, twice.

The slowdown is not uniform, which is what makes it look like a real cause rather than a slow runner. Timing the same two log landmarks from job start:

landmark	main	this branch
first test_mh_unit.py:3583	+10.8m	+19.0m
first test_l2tp_version_unit.py:116	+13.8m	+40.9m

So the segment between those two landmarks takes 3.0m on main and 21.9m here — about 7.3x. That segment is dominated by tests that call pcapkit.extract, and Extractor is what this change touches. #1025 adds guards in the same area and does not show it, so it is specific to this diff rather than to registrar guards generally.

I have not identified the mechanism yet and am not asserting one. Investigating now; review: needs-changes until it is explained or disproved. No code change requested yet, because the right fix depends on the cause.

[JarryShaw]

Correcting my own withdrawal: the slowdown is not attributable to this diff, and I should not have pointed at it. A profiling pass on a different model came back MECHANISM NOT FOUND, and I verified its load-bearing claims myself rather than taking the report.

What is refuted — that this code slows extract:

measurement	main fa3e861f5	this branch dae250597
cProfile function calls for one extract(http.pcap)	3,303,238	3,303,238
total time, same run	1.589 s	1.576 s
extract x60 with a full pcapkit purge/reimport per cycle	57.7 s	58.3 s
test_mh_unit + test_l2tp_version_unit, plain unittest, x2	59.6/9.0 s, 60.0/9.05 s	60.6/8.2 s, 58.9/8.15 s

The cumulative profile tops match line for line; the only differences are extraction.py line numbers shifted by the added code.

Why there is no mechanism available, which I checked directly. The three added methods are called from exactly one place each — the tail of their own public register_* at extraction.py:459, :528 and :597. Those run zero times during an extract: the built-ins are ModuleDescriptor literals in class-body dicts and never pass through a registrar at all. So the narrowed issubclass guards cannot be on a per-packet path, and the profiles show no issubclass or ABC cost on either side.

Corroboration from the same CI run I drew the original claim from: every sibling Plain unittest ordering leg was normal on this head, including foundation at 5.7 min — and foundation is the leg that actually exercises Extractor and the tests this diff modifies.

What my earlier evidence was worth. The CI durations are real — 44.9 m and 42.2 m here against 18-27 m elsewhere — and so is the widening landmark gap. What was wrong was the attribution. One detail undercuts the picture further: the contaminated local driver run that preceded this showed test_http_unit at ~2811 s on both trees, with the host's load average at 169, so that signal was host stall rather than a tree difference. I also won't lean on the siblings' earlier start times as evidence, as tempting as it looks — those legs ran in attempt 1 while protocols is my single-job re-run, so the gap is an artifact of how I re-ran it.

Still unexplained, and the reason I am not simply declaring this closed: two attempts on this branch were both slow. I have launched a third run of that leg as the direct test. If it lands in the 18-27 min band the cause was runner-side; if it comes back at ~42 min again, something real survives local profiling and I will say so.

Restoring review: good-to-go, since the cross-review verdict on this head was always about correctness and my withdrawal rested on a performance claim that has not held up. That is separate from the red check: I will not report this as ready to merge until the leg is green.

One measurement gap named rather than buried: the full protocols leg was never run locally in one process, because this host OOM-killed a 56 GB pytest run earlier today. So a slowdown that only appears cumulatively across the whole leg is not ruled out — which is exactly what the third CI attempt tests.