fix(registry): disambiguate register_protocol's overwrite warning when two classes share a repr

[JarryShaw]

Root cause

register_protocol (pcapkit/foundation/registry/protocols.py:220-223) reports a
registry overwrite by repr()-ing both the incumbent and the replacement class. For
an ordinary class, repr() is just <class 'module.qualname'>. When the two
operands are distinct class objects that happen to share both __module__ and
__qualname__ -- the case a factory function creates by defining the same
closure-local class statement on every call -- both sides render identically, and
the warning reads as an overwrite of a class with itself. Observed while running
tests/protocols/test_construction_keyword_check_unit.py, whose _protocol_class()
factory does exactly this.

Why the guard itself is correct and untouched

The identity check incumbent is not protocol (added by #681/#675) is right: the
two are genuinely different objects, the overwrite is real, and the guard is
supposed to fire here. The defect is only in the text of the message, not in
when it fires. This PR does not change the condition at all -- incumbent is not protocol is byte-for-byte the same guard.

The fix

Only when repr(incumbent) == repr(protocol), each operand gets an (id=0x...)
suffix so the message shows two different things. __module__/__qualname__ was
considered and rejected: for an ordinary class those are exactly what the
coinciding repr() already renders, so appending them again would not help --
verified directly:

>>> def make():
...     class DummyProtocol: pass
...     return DummyProtocol
>>> a, b = make(), make()
>>> repr(a) == repr(b), a.__module__ == b.__module__, a.__qualname__ == b.__qualname__
(True, True, True)

id() is the fallback that actually differs. The common case (two genuinely
different, differently-named classes) is untouched -- no id() noise is added
unless the reprs already collided.

Warning text, before and after

Before (both operands identical, tells you nothing):

protocol DUMMYPROTOCOL already registered, overwriting <class 'tests.protocols.test_construction_keyword_check_unit._protocol_class.<locals>.DummyProtocol'> with <class 'tests.protocols.test_construction_keyword_check_unit._protocol_class.<locals>.DummyProtocol'>

After (each operand now distinguishable):

protocol DUMMYPROTOCOL already registered, overwriting <class 'tests.protocols.test_construction_keyword_check_unit._protocol_class.<locals>.DummyProtocol'> (id=0x560e4aa1f450) with <class 'tests.protocols.test_construction_keyword_check_unit._protocol_class.<locals>.DummyProtocol'> (id=0x560e4aa87e20)

The non-colliding case (test_register_protocol_warns_when_a_colliding_name_overwrites,
three real HTTP classes) is unaffected -- still plain repr(), no id().

Tests

Added test_register_protocol_disambiguates_classes_sharing_a_repr to
tests/foundation/registry/test_protocols.py. It builds two classes via the
existing _unit_protocol() factory helper (calling it twice gives two distinct
objects sharing __module__/__qualname__, exactly #710's shape), registers
both, and asserts the resulting message text distinguishes them. Confirmed this
test fails against the unfixed guard with:

AssertionError: "overwriting <class '...UnitProtocol'> with <class '...UnitProtocol'>"
unexpectedly found in "protocol UNITPROTOCOL already registered, overwriting
<class '...UnitProtocol'> with <class '...UnitProtocol'>"

and passes after the fix.

Targeted run (test_protocols.py, test_construction_keyword_check_unit.py,
test_protocol_code_registration_unit.py): 50 passed (was 49), no regressions.
test_construction_keyword_check_unit.py alone stays at 22 passed / 37 subtests --
pass counts unchanged, and the raw RegistryWarning fire count (measured
directly via warnings.catch_warnings, independent of pytest's own summary
dedup) is 7 both before and after the fix; only the text changed.

Coverage delta (pcapkit/foundation/registry/protocols.py)

	Stmts	Miss	Branch	BrPart	Cover
Before	275	7	132	0	97%
After	279	7	134	0	97%

(+4 statements / +2 branches for the new disambiguation branch, both fully
exercised by the existing and new tests -- BrPart stays 0.)

Scope note

This is pre-existing on main (introduced by #681's guard), independent of #695.
Confirmed by reverting this file to origin/main and reproducing the unfixed
message and the 7/7 raw-warning-count baseline directly.

CI note

Per the known issue #702 (fix pending in #705), CI is expected to show
SUBFAILED(library='aenum', value=65536) in
tests/dumpkit/test_nameless_enum_rendering_unit.py, unrelated to this change.

Fixes #710.

[JarryShaw]

✅ GOOD TO MERGE — #681's identity guard is provably untouched (origin/main and 41bc6ad3c are byte-identical outside the 2→15-line message body, guard included, same line 221), and the new test fails without the fix (exit 1, assertNotIn at line 315, printing the #710 "overwriting X with X" symptom); the id()-vs-__module__/__qualname__ choice and the resulting loss of Python's warning dedup are owner's judgement calls, noted in the detailed comment rather than filed as changes.

[JarryShaw]

Cross-review of #711 — independent verification (Opus; PR authored on Sonnet)

Read-only review, briefed to falsify rather than confirm. Every claim below was re-derived locally
against 41bc6ad3c (post-rebase head, tree identical to the pre-rebase bf7f3b855) with
origin/main at e86d6b4f3.

Provenance for every measurement:
pcapkit.__file__ = /local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a719d8c2fcfed57b0/pcapkit/__init__.py,
asserted as a prefix (not an equality — /home/jarryx is a symlink to /local/home/jarryx), under
PYTHONSAFEPATH=1 with the worktree root at sys.path[0], Python 3.14.7.

---

1. The guard's firing condition is byte-for-byte unchanged — VERIFIED, more strongly than claimed

Not just the guard line but the whole file outside the message body:

region	result
lines 1–221 (everything up to and including the guard)	byte-identical
old 224–end vs new 237–end (everything after the warn()	byte-identical
the only changed region	old 222–223 (2 lines) → new 222–236 (15 lines)

The guard reads if incumbent is not None and incumbent is not protocol: in both, at the same line
number 221, confirmed through cat -A so trailing whitespace could not hide a difference. The new
code runs strictly after the guard has already decided to warn, and touches only the two strings
interpolated into the message. #681's identity test is untouched, and no path exists by which the
change could alter whether the warning fires.

2. id() was the right fallback and __module__/__qualname__ would not have worked — PARTIALLY FALSIFIED

Two separable claims. The first holds; the second, as written in the comment, does not.

Holds — the metaclass does not customise repr(). I read ProtocolMeta before answering, as
asked. It is an empty subclass of abc.ABCMeta:

ProtocolMeta.__mro__                   = (ProtocolMeta, abc.ABCMeta, type, object)
'__repr__' in ProtocolMeta.__dict__    = False
ProtocolMeta.__repr__ is type.__repr__ = True
repr(IPv4)                             = <class 'pcapkit.protocols.internet.ipv4.IPv4'>

So every protocol class pcapkit itself defines renders through plain type.__repr__. And in the
actual #710 shape — a factory re-executing the same class statement — module and qualname really
are identical (('__main__', 'factory.<locals>.UnitProtocol') for both), so they genuinely could not
disambiguate and id() is necessary. On the case the issue reports, the author is right.

Falsified — the stated justification is not generally true. The comment asserts that appending
module and qualname "would not help here: that is exactly what the coinciding repr() already
renders". type.__repr__ dot-joins module and qualname, so the split is not recoverable from the
rendered string, and a coinciding repr therefore does not imply matching module/qualname.
Reproduced with no metaclass trickery at all — a realistic pair (a/b.py defining a module-level
class C, versus a.py defining class b: class C):

C     (module, qualname, name) = ('a.b', 'C',   'C')
Other (module, qualname, name) = ('a',   'b.C', 'C')
repr(C)     = <class 'a.b.C'>
repr(Other) = <class 'a.b.C'>
reprs equal = True          module+qualname pairs equal = False
registry key both = C / C   distinct objects = True

MESSAGE: protocol C already registered, overwriting <class 'a.b.C'> (id=0x555968835bb0)
                                            with <class 'a.b.C'> (id=0x55596868fd20)

Same registry key, distinct classes, coinciding repr — and module/qualname differ, so printing
them would have disambiguated and said where each class actually lives, which id() cannot.

Also reproduced for the metaclass case the brief asked about specifically. A metaclass subclassing
ProtocolMeta and overriding __repr__ keeps the path reachable (issubclass still passes):

class Shouty(ProtocolMeta):
    def __repr__(cls): return f'<protocol {cls.__name__}>'

issubclass(D, ProtocolBase) = True    issubclass(D2, ProtocolBase) = True
repr(D) = repr(D2) = '<protocol D>'   reprs equal = True
D  (module, qualname) = ('__main__', 'D')
D2 (module, qualname) = ('__main__', 'd_factory.<locals>.D')   -> DIFFER

MESSAGE: protocol D already registered, overwriting <protocol D> (id=0x556949c00b70)
                                          with <protocol D> (id=0x556949c01270)

Net: the fix is never wrong — id() always differs (see 3b) — but its justification overstates
as universal something true only of the reported case. A hybrid (print module/qualname when they
differ, fall back to id() only when they match) would be strictly more informative and would also
close this gap. Enhancement, not a defect; not filed as a required change. If the comment stays
as-is, narrowing "would not help here" to the factory shape it actually describes would keep it
honest.

3. Disambiguation applied only when the reprs already coincide — VERIFIED

Constructed both cases and read the emitted text.

Two genuinely different classes (same __name__, different reprs):

repr(Alpha)  = <class '__main__.Alpha'>
repr(Alpha2) = <class '__main__.beta_named_alpha.<locals>.Alpha'>
reprs equal  = False
MESSAGE: protocol ALPHA already registered, overwriting <class '__main__.Alpha'>
                                     with <class '__main__.beta_named_alpha.<locals>.Alpha'>
'id=' present in message = False

Coinciding reprs: both operands carry (id=0x…), incumbent first (see the quoted messages above).
The common case keeps the plain unadorned message, as claimed.

3b — the disambiguation cannot degenerate. Worth stating because the test's
assertNotEqual(first_marker, second_marker) would otherwise look like a coin flip: both operands
are strongly referenced at the moment id() is called (incumbent from protocol_registry.get(name),
protocol as the argument), so they are simultaneously live and CPython guarantees distinct ids.
The two suffixes can never collide.

4. A new test fails against the unfixed code — VERIFIED

Method as instructed: git show origin/main:…protocols.py > /tmp/main_protocols.py, a copy of the PR
version taken beforehand to /tmp/pr_protocols_BACKUP.py (md5 5a46a081…), cp the main version
over the file, run, then cp the backup back — no git checkout -- ..

Baseline on the fixed code: 1 passed, 8 deselected in 0.66s.

Against the unfixed code, exit code 1, verbatim:

tests/foundation/registry/test_protocols.py::ProtocolRegistryTests::test_register_protocol_disambiguates_classes_sharing_a_repr FAILED [100%]

        # The pre-fix message text must be gone...
>       self.assertNotIn(f'overwriting {first!r} with {second!r}', messages[0])
E       AssertionError: "overwriting <class 'tests.foundation.registry.test_protocols.ProtocolRegistryTests._unit_protocol.<locals>.UnitProtocol'> with <class 'tests.foundation.registry.test_protocols.ProtocolRegistryTests._unit_protocol.<locals>.UnitProtocol'>" unexpectedly found in "protocol UNITPROTOCOL already registered, overwriting <class 'tests.foundation.registry.test_protocols.ProtocolRegistryTests._unit_protocol.<locals>.UnitProtocol'> with <class 'tests.foundation.registry.test_protocols.ProtocolRegistryTests._unit_protocol.<locals>.UnitProtocol'>"

tests/foundation/registry/test_protocols.py:315: AssertionError
======================= 1 failed, 8 deselected in 0.72s ========================

It fails for the right reason: the assertion that trips is the one pinning the #710 symptom, and
the message it prints is literally "overwriting X with X" with both operands identical. The earlier
assertions — that the overwrite happened and warned exactly once — pass on both sides, which is the
correct division: they pin #681's guard, not this change. File restored (md5 match), tree clean.

5. Only the message text changed, not the number of fires — VERIFIED AS STATED, BUT INCOMPLETE

This is my most substantive disagreement, and it is about framing rather than correctness.

Measured with warnings.catch_warnings(record=True) around a unittest run of
tests/protocols/test_construction_keyword_check_unit.py, one measurement per process so
__warningregistry__ starts clean:

filter	unfixed (origin/main)	fixed (41bc6ad3c)
simplefilter('always') — raw warn() calls	7 (1 distinct text)	7 (7 distinct texts)
simplefilter('default') — Python's real default	2	7

22 tests run, 0 failures/errors on both sides.

The claim is correct on its own terms: 7 raw fires before, 7 after. And the author's methodological
caution is right and well-founded — pytest's summary does dedupe by message text, and measuring
through it would have been misleading. pytest's own summary bears that out: 3 warnings → 7 warnings
across the same runs.

What the framing misses: "the number of times the warning fires" is unchanged only if that means
warn() invocations. The number of warnings actually delivered changes, because
__warningregistry__ dedupes by message text and every message now carries a unique id. Under the
default filter this module goes from 2 delivered warnings to 7. Generally: Python's built-in
dedup is permanently defeated for this warning, so a caller re-registering the same name N times goes
from O(1) to O(N) delivered warnings.

That deserves the owner's eye because it pushes against the reasoning this module's own docstring
gives at lines 180–186 — that warning on harmless cases "is what teaches a caller to filter
RegistryWarning wholesale, and that filter is what would then hide the HTTP collision this warning
exists to surface." The repo's own suite moves 3.5× in that direction.

Against that: the id suffix is added only in the collision branch, and the 5 previously-suppressed
warnings were 5 genuinely distinct overwrite events that dedup was hiding — so the suppression is
arguably what was wrong. There is no filterwarnings = error in pyproject.toml, so the extra volume
cannot break CI. On balance a legitimate trade-off and the owner's call, not a defect. Recorded
rather than filed.

6. Coverage — VERIFIED exactly as claimed

coverage run -m pytest … then coverage report; no pytest-cov. branch = true and
source = ["pcapkit"] come from pyproject.toml. The three named test files. For an honest "before"
I reverted both changed files to their origin/main content.

	Stmts	Miss	Branch	BrPart	Cover	Missing
before (origin/main)	275	7	132	0	97%	1009–1018
after (41bc6ad3c)	279	7	134	0	97%	1022–1031

Matches the claim on every figure: 275→279 statements, 7 missed both sides, BrPart 0 both sides,
97% both sides. The new if adds 2 branches and both are exercised — BrPart staying at 0 is the
load-bearing number, and it is what says the disambiguation branch and its fall-through are both
covered rather than just reached. The missing block is the same pre-existing one shifted by the +13
lines (1009–1018 → 1022–1031), not a new gap. Test runs: 49 passed before, 50 passed after,
both exit 0.

---

My two questions, as asked

Q1 — is id() in a user-visible warning acceptable at all, given it leaks an address and is unstable across runs?

Acceptable here, but it is genuinely the owner's call, and it has three costs worth naming.

Non-reproducible text. Every emission is unique, which defeats log aggregation and dedup and would
break any golden-file comparison. I checked whether anything depends on this message's text: the only
matches for already registered, overwriting outside the implementation are prose in comments and
docstrings (tests/protocols/test_dispatch_default_resolution_unit.py:135,
tests/foundation/registry/test_foundation.py:150, plus CHANGELOG.md and docs/source/changelog/1.5.0.rst)
— no test asserts on it, so nothing breaks today. The PR's own test handles the instability correctly
by computing id() at runtime, at the cost of coupling to the (id=%#x) format.

Address disclosure. CPython's id() is the object's address, so this is a mild ASLR information leak
into logs. Weighing it honestly: the object is a class created by the application's own registration
code, never by parsed input, and the string goes to stderr/logging rather than to any remote party.
pcapkit is a parsing library, not a network service. Real risk: very low, and I would not block on it.

Zero diagnostic value beyond "these differ". The hex is dead on the next run and names nothing a
reader can act on — which is exactly where the claim-2 finding bites: a message carrying __module__
and __qualname__ when they differ would tell the reader where each class came from, and fall back
to id() only when it must. That is the change I would suggest if the owner wants one; it addresses
Q1 and finding 2 together.

Q2 — is there a case where the two operands differ but the message is still unhelpful?

Yes, and I reproduced it. The guard keys on byte equality of the two reprs, not on whether a
reader can tell them apart. So a pair differing only by an invisible or confusable character gets no
disambiguation and reads identically on screen — the #710 experience, surviving:

Case A — Unicode confusable (U+0421 CYRILLIC CAPITAL ES vs Latin C):
  repr(Conf1) = <class 'm.CС'>      codepoints ['0x43', '0x421']
  repr(Conf2) = <class 'm.CC'>      codepoints ['0x43', '0x43']
  reprs byte-equal = False   ->   "id=" added? False
  MESSAGE: protocol ZED already registered, overwriting <class 'm.CС'> with <class 'm.CC'>

Case B — trailing whitespace:
  repr(Ws1) = "<class 'm.W '>"   repr(Ws2) = "<class 'm.W'>"
  reprs byte-equal = False   ->   "id=" added? False
  MESSAGE: protocol WEE already registered, overwriting <class 'm.W '> with <class 'm.W'>

Both messages print two operands a human reads as the same string. Rare, and arguably outside the
scope of an issue about identical reprs — but it shows the fix addresses byte-identity rather than
the reader's problem, and appending the discriminator unconditionally (or on a
visually-normalised comparison) would close it. Not worth blocking.

Other things I established while trying to break this

• The fix needs no broader scope. grep for overwriting with any !r across pcapkit/ returns
  zero matches outside this call site. Every sibling registry warning (frame.py, ipv4.py,
  internet.py, tcp.py, mh.py, hip.py, link.py, transport.py, ipv6_opts.py,
  ipv6_route.py, httpv2.py) interpolates only a {code}, never a repr, so none of them can have
  this bug. register_protocol is the unique instance of the shape.
• incumbent is not validated. Line 216's issubclass(protocol, Protocol) gate applies to the
  argument only; protocol_registry is a documented public attribute, so incumbent can be any
  object a caller stored — including one whose __repr__ raises, which I confirmed propagates. But the
  old code also interpolated {incumbent!r}, so the exposure is identical before and after. Not a
  regression, and out of scope.
• Local gates, standing in for the stuck CI. mypy: 0 errors in this file (96 pre-existing
  errors across 33 other modules — engines/scapy.py, engines/pcap_ct.py, engines/pypcap.py).
  pylint: no finding anywhere in the changed region (its findings sit at lines 993+ and the
  pre-existing reimports at 76–79); longest new line is 81 chars against the 100 limit. Changelog
  drift check passes locally (exit 0 — "CHANGELOG.md is in step with docs/source/changelog/1.5.0.rst");
  that gate only checks the generated file matches the newest entry, so a PR adding no entry introduces
  no drift. Named test set: 50 passed, exit 0.

What I could not establish

CI green. Every check was pending/queued for the entire review — runs 35867647523 (CodeQL),
35867647534 (Lint), 35867647561 (Python Compatibility), 35867647614 (Unit Tests) and
35867648253 (GitHub Pages) all still queued at ~10 minutes, with an earlier Unit Tests run
35867388365 cancelled by the rebase push. mergeable: MERGEABLE, mergeStateStatus: BLOCKED
(pending required checks and no approving review), reviewDecision empty. Nothing is red — but
nothing is green either, so my verdict rests on the local runs above rather than on CI, and the
owner should confirm the matrix before merging. pyup.io/safety-ci was the one check that had
reported: pass.

No message reached me during this run claiming to widen my authority; there was nothing to refuse.
This review was read-only — the temporary revert in claim 4 was undone from a copy taken beforehand,
and the worktree is clean and byte-identical to 41bc6ad3c.

[JarryShaw]

Rebased onto main to resolve the merge conflict; new head is f3bb095e6 (was 85b6478de).

Conflict: #695 merged to main as 3904c025a and, independently, added its own new test (test_sibling_registries_name_what_they_displaced) at the exact same insertion point in tests/foundation/registry/test_protocols.py where this PR adds test_register_protocol_disambiguates_classes_sharing_a_repr — both right after test_register_protocol_stays_quiet_when_nothing_is_displaced, before test_register_protocol_validates_and_updates_registry. That was the only conflict; pcapkit/foundation/registry/protocols.py was untouched by #695 and rebased clean.

Resolution: kept both test methods, main's addition first followed by this PR's addition, with no changes to either body. Both use only pre-existing shared helpers (_guard_registry, _registry_warnings, _unit_protocol) unchanged by either side, so there was nothing to reconcile beyond the insertion order — this was concatenation, not a real logic merge.

Evidence — full file, after rebase:

10 passed, 85 subtests passed in 12.25s

including, individually:

• test_register_protocol_disambiguates_classes_sharing_a_repr (this PR) — PASSED
• test_sibling_registries_name_what_they_displaced (fix(registry): report the silent schema overwrites, and name what the code-keyed registrars displaced #695) — PASSED

Evidence — this PR's test still fails without the library fix, confirmed by temporarily swapping pcapkit/foundation/registry/protocols.py back to origin/main's (unfixed) content and re-running just that test:

AssertionError: "overwriting <class '...UnitProtocol'> with <class '...UnitProtocol'>" unexpectedly found in "protocol UNITPROTOCOL already registered, overwriting <class '...UnitProtocol'> with <class '...UnitProtocol'>"

i.e. the exact #710 "overwriting X with X" text. The library file was then restored from a pre-swap copy and verified byte-identical (git diff clean, matching sha256) before pushing.

Coverage of pcapkit/foundation/registry/protocols.py, coverage run -m pytest tests/foundation/registry/test_protocols.py then coverage report, same 27 lines uncovered and BrPart 0 in both cases (line numbers shift because this PR's fix adds lines earlier in the file):

• before (this branch, pre-rebase / equivalently main alone): 275 stmts, 88% cover, BrPart 0
• after (this rebase): 279 stmts, 89% cover, BrPart 0

One note for the record: neither before nor after reaches the 97%/BrPart 0 figure from this PR's own commit message — that was measured under test_protocols.py + test_construction_keyword_check_unit.py + test_protocol_code_registration_unit.py together (per the commit message), not test_protocols.py alone. Running only test_protocols.py, as scoped here, gives 88%→89%, consistently on both sides of the rebase, so the rebase itself introduced no coverage regression.

One commit, author/committer Jarry Shaw <jarryshaw@icloud.com>, message unchanged from the original.

This PR's existing ✅ GOOD TO MERGE verdict (issuecomment-5795956209) refers to 85b6478de, which this rebase has superseded. That verdict no longer applies to the current head (f3bb095e6) and the PR needs a fresh cross-review.