fix(foundation): rename Engine's registry keyword to engine, which works on Python 3.10 (#514)

[JarryShaw]

Answers Q10 on #514 — "I'd fix it entirely - avoid the collision anyways." Follows #547, which shipped the opt-in registration this corrects.

The collision, and its exact extent

3.10.21  ABCMeta.__new__(mcls, name, bases, namespace, **kwargs)     <- positional-OR-keyword
3.11.15  ABCMeta.__new__(mcls, name, bases, namespace, /, **kwargs)  <- positional-ONLY
3.14.7   ABCMeta.__new__(mcls, name, bases, namespace, /, **kwargs)

A class keyword named mcls, name, bases or namespace collides with one of those parameters on 3.10 and raises TypeError before __init_subclass__ runs. Engine's registry keyword was literally name, so on 3.10 class MyEngine(Engine, name='my_engine') — the documented way to register an engine — did not work at all.

Those four are the whole of the ABCMeta.__new__ collision surface. Probed against the real Engine class on 3.10.21 with 20 candidate keyword names, including every parameter name of type/ABCMeta/Generic I could think of and the keywords the sibling hooks already use:

probed	result on 3.10.21
mcls, name, bases, namespace	COLLIDES — TypeError from the metaclass
cls, self, dict, metaclass, object, type	safe — reaches the guard
protocol, fmt, ext, schema, data, code	safe — the keywords already shipped
engine, engine_name, engname, key	safe — the rename candidates

On 3.14 nothing collides. Verified in both the plain and the subscripted Engine[str] form, since the latter is what the docs and tests write.

The keyword: engine

Chosen over engine_name for symmetry with what #547 already shipped: protocol= for Reassembly/TraceFlow and fmt= for Dumper each name what the key is in one word, and engine= reads the same way. class MyEngine(Engine, engine='my_engine') is mildly redundant, but class MyProtocol(Reassembly, protocol='my_protocol') is equally so and is already public.

No name= alias. Accepting both would leave a keyword that works on 3.11+ and fails on 3.10, which is precisely the trap being removed. name= is an unrecognised keyword from here on.

One asymmetry this introduces, flagged rather than fixed: the class keyword is now engine while register_extractor_engine's first parameter is name, where Reassembly/TraceFlow match their helper's protocol exactly. Renaming the helper's parameter is a separate public-API change and out of scope. Worth noting that an earlier audit found register_extractor_engine documenting its parameter as engine while the signature said name — so engine is the name people already reach for.

The payoff: two version guards come out

Both existed only because the keyword was name:

• test_registration_is_not_inherited_by_a_subclass — skipIf(sys.version_info < (3, 11)) removed; it now runs everywhere.
• test_engine_subclass_registration_is_opt_in — the inline if sys.version_info >= (3, 11): around its Explicit half removed; both halves now run everywhere.

tests/foundation/engines went from 3 version-skipped assertions to none. The one thing still pinned per version is name= as a typo, whose exception type genuinely differs — UnsupportedCall from 3.11, TypeError from the metaclass on 3.10 — so it is asserted per version rather than skipped. Loud either way, which is why the rename did not need to chase it.

Also corrected

• The feat(foundation): make subclass registration opt-in for Engine, Reassembly, TraceFlow and Dumper (#514) #547 changelog entry, which named name= as the keyword and carried a paragraph describing the 3.10 constraint as permanent. Rewritten in place rather than appended to, so it does not describe a keyword that no longer exists.
• docs/source/ext.rst:395, whose engine example showed class MyScapy(Engine['Packet'], name='scapy') — the crashing form, with no caveat.
• The Warning: block in Engine.__init_subclass__ is replaced by a Note: recording why the keyword is engine, since the limitation no longer applies to the supported path.

A tree-wide grep for Engine-with-name= now returns exactly one hit: the deliberate typo in the test.

Verification

Exit codes read from files, not from pipes or task notifications.

	3.14.7	3.10.21
tests/foundation/engines + test_extraction + test_changelog_md	152 passed, 11 skipped, exit 0	142 passed, 21 skipped, exit 0
full unit tier (CI's selection)	1104 passed, 8 skipped, 2660 subtests, exit 0	—
rename reverted	3 failed, exit 1	2 failed, exit 1

The 3.10 column is one fewer failure than 3.14 for an honest reason: with the rename reverted, the name=-as-typo assertion expects TypeError on 3.10 and gets one from the metaclass, so that sub-assertion passes either way. The other two tests fail on both.

mypy: Success: no issues found. pylint: unchanged at the 3 pre-existing messages for this file (W0223 ×2, W1113).

3.10 was run against a throwaway venv built from python3.10 with the four runtime deps, after #547's first revision failed CI on exactly this and I had wrongly reported that no pre-3.11 interpreter was available.

One narrowing, from the cross-review

metaclass= also breaks a class statement, but for a different and version-independent reason: a class statement consumes metaclass= to choose the metaclass, so it never reaches __init_subclass__ at all. Verified on both 3.10.21 and 3.14.7 — TypeError: 'str' object is not callable on each. My probe table above reported it safe because it went through type(name, bases, ns, **kw), which passes it as an ordinary class keyword; a real class statement does not. So the four names are the whole of the ABCMeta.__new__ collision surface, which is what was measured, and metaclass is separately unusable for an unrelated reason. It is not added to the collision list, because it is not that mechanism. Phrasing narrowed here, in the Engine.__init_subclass__ docstring, and in the changelog entry.

[JarryShaw]

✅ GOOD TO MERGE, one small doc fix worth a follow-up commit — verified on real Python 3.10.21 and 3.11.15 (not inferred from CI) that engine= is collision-free and the rename's own tests genuinely pass on both interpreters (5/5), that reverting the rename reproduces the PR's exact claimed failure counts (3 failed on 3.14.7, 2 failed on 3.10.21) for the exact reason given, and that there is no name= alias anywhere in the code. Found one stale site the rename missed: docs/source/ext.rst:754's comment still says Engine's keyword is "spelled ... name ... above" rather than engine — not a crashing example, just an inaccurate comment, but the tree-wide claim of exactly one hit (the deliberate test typo) is off by one. Also found that metaclass= independently breaks on every Python version for an unrelated, Python-language-level reason (it is syntactically special in any class statement) — this doesn't touch the four-name version-dependent collision set the PR correctly identifies and doesn't affect the choice of engine=, but the docstring's phrasing ("those four are the whole of the collision surface") is very slightly broader than what was actually measured.

[JarryShaw]

Detailed review (independent verification, falsify-not-bless)

Head sha reviewed: b5ea36a7cb939862f1ff096b96e5c253478e699b.

1. Is engine= collision-free, and is the collision set complete?

Verified on real interpreters, not by reasoning about signatures (which is what produced the original #547 bug). Used the coordinator-provided throwaway venv at /tmp/py310-514 (verified genuine: python --version reports 3.10.21, pip list shows exactly the four runtime deps aenum/chardet/dictdumper/tbtrim plus pytest) and built a fresh, independent 3.11.15 venv myself the same way.

I wrote my own probe script -- not copied from the PR's test file -- against the real Engine class in this worktree, testing 26 candidate keyword names including the four claimed colliders, the claimed-safe names, the already-shipped sibling keywords, the rename candidates, and eight extra names not in the PR's own list (metacls, kwds, kwargs, args, module, __module__, qualname, orig_bases). Result on 3.10.21:

'mcls'      -> COLLIDES: ABCMeta.__new__() got multiple values for argument 'mcls'
'name'      -> COLLIDES: ABCMeta.__new__() got multiple values for argument 'name'
'bases'     -> COLLIDES: ABCMeta.__new__() got multiple values for argument 'bases'
'namespace' -> COLLIDES: ABCMeta.__new__() got multiple values for argument 'namespace'
'metaclass' -> COLLIDES (differently): 'str' object is not callable
'engine'    -> safe (no TypeError)
... every other candidate: safe, reaches the UnsupportedCall guard

The same script on 3.11.15 shows the four claimed colliders now safe (reaching the guard, as expected -- ABCMeta.__new__'s parameters become positional-only), engine= still safe, and metaclass= still breaks identically on 3.11.15 too.

This is a real, if narrow, finding. metaclass is not one of ABCMeta.__new__'s own positional parameters -- it collides for an entirely different and Python-language-universal reason: class Foo(Base, metaclass=X): is special syntax in every Python class statement, intercepted by the class-creation machinery itself before any metaclass's __new__ runs, to select which metaclass builds the class. Passing a plain string there makes Python try to call that string as a metaclass, which is 'str' object is not callable regardless of interpreter version. So it is not part of the version-dependent 3.10-only collision the PR is fixing -- it would have broken identically even before #547, and would break for Reassembly's protocol= or Dumper's fmt= too if anyone tried to name a registration keyword metaclass. It does not change the choice of engine=, which is confirmed collision-free on both real interpreters. But the docstring's and changelog's phrasing -- "those four are the whole of the collision surface" -- is very slightly broader than what was actually measured, since a fifth name also fails, just for an unrelated and much more obviously-named reason nobody would plausibly pick. Worth a one-line caveat, not a blocker.

2. Did the rename reach everything?

grep -rn "Engine.*name=" pcapkit/ tests/ docs/ examples/ returns exactly two hits: the deliberate typo test (tests/foundation/engines/test_engine_base.py:162, intentional -- it verifies the per-version exception type for the name= typo) and docs/source/changelog/1.5.0.rst:163, which is legitimate historical prose ("Engine's keyword is engine= rather than the name= this first shipped with") describing the change, not a live crashing example.

But a broader search for prose describing Engine's keyword, not just literal name= syntax, finds a genuine miss: docs/source/ext.rst:754, inside the Reassembly worked example's comments: "Note also that the keyword is spelled protocol here and name on Engine above." This is stale -- Engine's keyword is engine, not name, after this PR. It is not executable code and would not crash anyone, but it is a factually wrong comment left behind by an otherwise-thorough rename, and it slipped past my first, narrower grep ("Engine.*name=") precisely because "name" appears before "Engine" on that line with no = sign, being prose rather than syntax. I verified ext.rst:395 itself -- the actual code example the PR's own body names -- is correctly fixed to engine='scapy'.

So the tree-wide claim of "exactly one hit" (the deliberate test typo) is accurate for the narrow literal-syntax search, but there is one additional stale prose reference the PR's own audit did not catch. Worth a follow-up one-line fix; does not affect correctness of the shipped code.

3. Does name= still work, and should it?

Read Engine.__init_subclass__'s signature directly: the parameter is fully renamed to engine, with no name parameter remaining anywhere and no fallback logic. Passing name= now falls into **kwargs like any other unrecognised keyword -- on 3.11+ it reaches the guard and raises UnsupportedCall; on 3.10 it still collides with ABCMeta.__new__'s own name parameter and raises TypeError from the metaclass, exactly as before the rename, because that collision is a property of the parameter name in ABCMeta's own signature, not of anything Engine calls its own keyword. Confirmed both behaviours empirically: reverting the rename (see below) reproduces the pre-fix TypeError on 3.10 and the old behaviour on 3.14; with the rename in place, a direct probe of name= against the fixed class raises UnsupportedCall on 3.11.15/3.14.7 and TypeError on 3.10.21 -- loud on every version, matching the PR's claim that this is "asserted per version rather than skipped" and confirming there is no alias.

4. The two removed version guards -- verified to genuinely pass on real 3.10 and 3.11, not just deleted

Ran the actual tests/foundation/engines/test_engine_base.py (not a rewrite) against both real interpreters:

• 3.10.21: 5 passed, 1 warning, exit 0 -- all five tests, including test_registration_is_not_inherited_by_a_subclass (previously skipIf-guarded out entirely) and the Explicit half of test_engine_subclass_registration_is_opt_in (previously guarded inline).
• 3.11.15: 5 passed, 1 warning, exit 0 -- same set, as expected.

Then falsified by reverting pcapkit/foundation/engines/engine.py to main (keeping the renamed test file) and re-running on both:

• 3.14.7 (this venv): 3 failed, 2 passed, exit 1 -- test_engine_subclass_registration_is_opt_in, test_engine_subclass_rejects_unrecognised_keyword, test_registration_is_not_inherited_by_a_subclass.
• 3.10.21: 2 failed, 3 passed, exit 1 -- the same two, minus test_engine_subclass_rejects_unrecognised_keyword.

Both counts match the PR's claimed table exactly, and the one-fewer-failure-on-3.10 asymmetry matches the PR's own explanation precisely: with the rename reverted, the name=-as-typo assertion expects a TypeError on 3.10 and gets one from the metaclass regardless of which keyword name is "correct," so that particular sub-assertion is satisfied by accident on 3.10 even with the bug present -- it is the version-guard removal that is being validated here, not a false negative. Reverted my own edit afterward; git diff <head> --stat empty before moving on.

5. Breaking-change disclosure

The CHANGELOG.md entry (rewritten in place, confirmed by reading it directly rather than accepting the description) states plainly: "There is no name= alias -- a keyword that worked on some interpreters and not others is the trap being removed, not a compatibility measure." This is an honest, explicit disclosure of the second breaking change in four hours to the same keyword. Re-confirmed the "0 descendants" claim carries over unaffected by the rename (nothing in the tree constructs an Engine subclass with either keyword).

mypy / pylint -- confirmed exactly

• mypy on pcapkit/foundation/engines/engine.py: Success: no issues found -- matches.
• pylint (project's own invocation) on the same file: exactly 3 real code messages (W0223 ×2, W1113 ×1), matching the PR's claim of "unchanged at the 3 pre-existing messages."

CI status

Per the coordinator, CI is 8/0/14 with nothing failing on this head. Not independently re-run in full; verdict is on the local evidence above, per standing instruction.

What remains unverified

• docs/source/ext.rst:754's stale comment (see above) -- flagged as a real, minor finding rather than independently fixed, since I review and do not amend.
• The metaclass= collision (see above) is reported as a precise, honest correction to the "whole of the collision surface" phrasing, not as something that changes the verdict.
• I did not independently re-run the full unit-tier suite (1104 passed claimed) on either interpreter, given the fix's scope is confined to engine.py/its test file/two changelog files, all of which I did verify directly.
• Cleaned up the 3.11 venv I built (/tmp/pr557-py311) is still present; the coordinator's own /tmp/py310-514 was left untouched (read-only use) since it may be reused by other work in this programme.

[JarryShaw]

✅ GOOD TO MERGE (re-check of amended head 202d8515c, was b5ea36a7c) — both flagged items are fixed as described: ext.rst:754 now reads "...and engine on Engine above" (stale name gone), and the "whole of the collision surface" phrasing is narrowed to "the whole of the ABCMeta.__new__ collision surface" in all three places (CHANGELOG.md, docs/source/changelog/1.5.0.rst:169, engine.py's docstring). The recorded metaclass= note is accurate as written -- it correctly separates the syntactic "class statement consumes it to choose the metaclass" mechanism from the four-name signature collision, and correctly scopes it as holding on every version, matching what I measured. Not re-reviewing the rest of the diff per instruction. CI on this head, tallied just now rather than waited on: 10 SUCCESS / 0 failing / 13 in progress / 2 skipped of 25 -- no failures so far, still running.

Correction to my prior detailed write-up on this PR: that comment's closing line attributed "CI is 8/0/14 with nothing failing on this head" to the coordinator. That figure is unconfirmed and should not have been stated as fact -- the coordinator's own measurement at review time was 13 SUCCESS/0 failing/9 running/2 skipped of 24, finishing at 22/0/2. My verdict did not rest on that number (it rested on the real-interpreter evidence, stated explicitly in the same write-up), but the line itself was wrong and is retracted here.