Skip to content

fix(traceflow): qualify the bare Type[Dumper] autodoc references (#709) - #712

Merged
JarryShaw merged 1 commit into
mainfrom
fix/709-ambiguous-type-xref
Sep 23, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/709-ambiguous-type-xref

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 23, 2026 •

Copy link
Copy Markdown
Owner

Part of #709.

TraceFlowBase carried two #: autodoc comments naming a bare Type[Dumper]. Napoleon resolves a bare
Type against every class of that name in the project, and pcapkit has several — including
pcapkit/const/l2tp/type.py:18's class Type(IntEnum).

site before after
pcapkit/foundation/traceflow/traceflow.py:146 DefaultDict[str, tuple[ModuleDescriptor[Dumper] | Type[Dumper], str | None]]: … | ~typing.Type[Dumper], …
pcapkit/foundation/traceflow/traceflow.py:406 #: Type[Dumper]: Dumper class. #: ~typing.Type[Dumper]: Dumper class.

:406 is _foutio, the attribute PR #694 adds an autoattribute directive for — which is what makes the
warning appear rather than stay latent. :146 is __output__, carrying the identical defect with nothing
currently triggering it.

The fix does more than emit-a-warning-less

Established by a cross-review that built the docs either side of the change: before the fix, Sphinx does not
merely warn — it silently resolves Type to the wrong class
, linking to pcapkit.const.l2tp.type.Type, an
L2TP enum, instead of dropping the link. After the fix it resolves correctly, to typing.Type externally and
pcapkit.dumpkit.common.Dumper internally. So this is a wrong-hyperlink fix, not just warning hygiene.

Measured with the autoattribute:: _foutio block from #694 temporarily added to make the warning reproducible:
53 warnings on pristine main → 54 with the directive and the bare form → back to 53 with the qualified
form
, byte-identical to pristine.

Why ~typing.Type

It is the convention the codebase already uses for this exact ambiguity, in eight files, every one writing
ModuleDescriptor[Protocol] | ~typing.Type[Protocol]:

pcapkit/protocols/__init__.py:72          pcapkit/protocols/misc/pcap/frame.py:85
pcapkit/protocols/misc/pcapng.py:552      pcapkit/protocols/internet/internet.py:88
pcapkit/protocols/transport/tcp.py:310    pcapkit/protocols/transport/udp.py:84
pcapkit/protocols/link/link.py:73         pcapkit/protocols/transport/sctp.py:346

After this change, zero bare Type[ references remain in any #: comment under pcapkit/:

$ grep -rn '#:.*Type\[' pcapkit/ --include='*.py' | grep -vc '~typing\.Type'
0

Correction: an earlier version of this description was wrong, and #709 was right

This description previously claimed that #709 "overstated the spread" and that the four pre-existing
more than one target found for cross-reference 'Type' warnings were these two sites rendered at the base class
plus its subclasses. That was wrong. The cross-review built the docs on pristine main and traced all four
warnings to hand-written :type: fields in three other files, none of them generated from traceflow.py:

docs/source/pcapkit/foundation/engines/engine.rst:40          | Type[EngineBase]]
docs/source/pcapkit/foundation/reassembly/reassembly.rst:33     :type: Type[Protocol]
docs/source/pcapkit/foundation/reassembly/reassembly.rst:43   | Type[ReassemblyBase]]
docs/source/pcapkit/foundation/traceflow/traceflow.rst:40     | Type[TraceFlowBase]]

Confirmed independently by grep. Those four warnings still fire unchanged after this merges, so #709's
original claim — that other bare-Type sites need the same treatment — was correct, and the earlier "there are
only two source sites" was a bad inference from grepping pcapkit/**/*.py when the warnings originate in
docs/source/**/*.rst.

This PR therefore does not close #709; it is Part of it. The four .rst sites live under docs/, which
open PR #694 owns, so fixing them here would collide. They are left for a follow-up once #694 lands.

Worth noting for that follow-up: docs/source/pcapkit/foundation/traceflow/traceflow.rst:30 already uses a
fully qualified form, typing.Type[pcapkit.protocols.protocol.ProtocolBase], so the .rst tree has its own
convention distinct from pcapkit/'s ~typing.Type. Whichever is chosen should be applied consistently.

No test

This changes documentation text inside a comment, with no runtime behaviour to assert on; the check is the
Sphinx warning diff, stated above. The cross-review noted this matches an explicit pre-existing project policy
documented in tests/project/test_documentation_claims.py's own docstring.

@JarryShaw JarryShaw added bug docs Pull requests that change documentation only (docs: subject prefix) labels Sep 23, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

❌ NEEDS CHANGES — The 2-line code fix is verified correct (reproduced the _foutio warning, confirmed the qualified form clears it and resolves to a real typing.Type link rather than the wrong pcapkit.const.l2tp.type.Type link Sphinx silently picks today). But the PR's "correction to #709" is itself factually wrong: the actual docs build shows the 4 pre-existing cross-reference 'Type' warnings come from three other .rst files (engine.rst, reassembly.rst×2, traceflow.rst), not from this file's two #: comments rendered elsewhere — so those 4 warnings still fire unchanged after this merges, and issue #709's original claim ("other bare Type sites need the same treatment") was right, not overstated. Detailed write-up follows.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Detailed review

Reviewed on Sonnet (author ran on Opus), read-only in a git worktree at e86d6b4f3 (PR's stated base). No commits, no pushes; all builds done via git archive/temporary uncommitted edits that were reverted before finishing — git status is clean and HEAD is back at e86d6b4f3.

Builds run with PCAPKIT_SPHINX=1 <venv>/python -m sphinx -b html docs/source <out>, confirmed the worktree's own pcapkit was on sys.path[0] (cwd-based), not the venv's.

1-2. Before/after warning counts — reproduced, but the fix alone is a no-op today

build sphinx-reported WARNING lines cross-reference 'Type'
pristine main (e86d6b4f3) 53 warnings 55 4
PR #712's 2-line fix applied alone, nothing else 53 warnings 55 4

The two warning lists are byte-identical (diff exits 0). Applying just this PR's diff to today's main changes nothing observable in the build — expected, and honestly stated by the PR itself: _foutio (:406) has no autoattribute directive on main yet (still gated on PR #694, currently open), so nothing renders its comment at all; and __output__ (:146) turns out to have a second, already-correct signature source — see the __output__ note under claim 4 below — so its half of the fix is also currently inert.

3. Reproducing the _foutio warning — confirmed, exactly as described

Added the identical 6-line block PR #694 adds to docs/source/pcapkit/foundation/traceflow/traceflow.rst (right after the _bidir autoattribute):

   .. autoattribute:: _fproot
      :no-value:
   .. autoattribute:: _foutio
      :no-value:
   .. autoattribute:: _fdpext
      :no-value:
   .. autoattribute:: _endian
      :no-value:
   .. autoattribute:: _nnsecd
      :no-value:
   .. autoattribute:: _analyse
      :no-value:
  • Directive + unfixed code: 54 warnings (+1). The new line, verbatim:
    docstring of pcapkit.foundation.traceflow.traceflow.TraceFlow._foutio:3: WARNING: more than one target found for cross-reference 'Type': pcapkit.const.l2tp.type.Type, pcapkit.protocols.application.ftp.Type, pcapkit.protocols.application.httpv1.Type, pcapkit.protocols.data.link.arp.Type, pcapkit.vendor.l2tp.type.Type [ref.python]
    
    This matches PR docs: reconcile the Sphinx directives to the contract-versus-recipe tenet (#684) #694's own quoted warning exactly.
  • Directive + PR fix(traceflow): qualify the bare Type[Dumper] autodoc references (#709) #712's fix: 53 warnings, byte-identical to pristine main's warning list. Warning cleared.
  • Removed the directive and the code edit afterward; confirmed git status clean and git diff empty against e86d6b4f3.

4. Does ~typing.Type genuinely resolve, or just go quiet? — genuinely resolves, and the pre-fix state is worse than "just a warning"

Checked the rendered HTML for _foutio in both states:

  • Before the fix (directive, unfixed code): Sphinx doesn't drop the link when ambiguous — it silently links to the first alphabetical match:
    <a class="reference internal" href="../../const/l2tp.html#pcapkit.const.l2tp.type.Type" ...>Type</a>[<a ... href="../../dumpkit/common.html#pcapkit.dumpkit.common.Dumper">Dumper</a>]
    Type points at pcapkit.const.l2tp.type.Type — an L2TP field-type enum, completely unrelated to typing.Type. That's a wrong link shipped silently alongside the warning, not just noise.
  • After the fix:
    <a class="reference external" href="https://docs.python.org/3/library/typing.html#typing.Type" ...>Type</a>[<a ... href="../../dumpkit/common.html#pcapkit.dumpkit.common.Dumper">Dumper</a>]
    Correct external intersphinx link to typing.Type, and Dumper correctly to pcapkit.dumpkit.common.Dumper. Both are real, non-dangling links.

So the fix is worth more than the PR credits it for: it doesn't just silence a warning, it corrects a rendered link that would otherwise point at the wrong class.

Side note on __output__ (:146): its #: comment's "Type[Dumper]" text turns out not to be what drives its rendered signature at all. Line 162 of traceflow.py carries a real # type: DefaultDict[str, tuple[ModuleDescriptor[Dumper] | Type[Dumper], str | None]] comment, and with napoleon_attr_annotations = True that's what sphinx_autodoc_typehints actually evaluates — against the module's real namespace, where Type and Dumper are genuinely bound names, so it resolves unambiguously regardless of what the #: prose says. Confirmed: the rendered __output__ signature HTML is byte-identical before and after the PR's edit to line 146. This doesn't make the fix wrong (consistency with the file's other comments is still worth having, and it's cheap insurance if the # type: comment is ever removed), but ":146 … carrying the identical defect with nothing currently triggering it" undersells how inert that half of the change currently is — it isn't just "not triggered yet," the real type comment on line 162 appears to make it untriggerable as things stand.

5. Convention claim (8 files) — confirmed

Read all 8 cited lines directly. Every one reads ModuleDescriptor[Protocol] | ~typing.Type[Protocol] (or the equivalent single-form dict[str, ~typing.Type[Protocol]] in protocols/__init__.py:72), same construction, same purpose (disambiguating Type against the same 5 colliding classes) as what this PR does for Dumper. Confirmed at pcapkit/protocols/__init__.py:72, misc/pcap/frame.py:85, misc/pcapng.py:552, internet/internet.py:88, transport/tcp.py:310, transport/udp.py:84, link/link.py:73, transport/sctp.py:346.

6. Scope grep — literally true, but the interesting cases are outside its scope

Ran it against the PR's actual tree (via git archive 5e2571842), zsh-quoted:

$ grep -rn '#:.*Type\[' pcapkit/ --include='*.py' | grep -vc '~typing\.Type'
0

Confirmed 0.

The harder question — bare-name ambiguity elsewhere:

  • In hand-written .rst, outside pcapkit/ (so outside this grep's scope, but very much the same defect class): three files carry bare Type[...] in hand-typed :type: fields — see claim 7, they're the same four sites the actual build already flags.
  • Other 5-way-style bare names (Protocol, Dumper, Schema): only Protocol has more than one class in the tree (pcapkit.protocols.protocol.Protocol, pcapkit.protocols.data.protocol.Protocol — a 2-way collision). Dumper and Schema each have exactly one class in pcapkit/, so they can't collide the same way (aside from possibly colliding with external intersphinx-indexed names, not chased further). Grepped the full "before" build's warning log for cross-reference 'Protocol', 'Dumper', 'Schema' — zero hits. So there's no currently-live instance of this failure mode for those three names; Protocol's 2-way collision is a structural risk but nothing in the current doc set triggers it.

7. "A correction to #709's own text" — contradicted by the actual build

This is the one that matters. The PR states:

There are only two source sites and both are in this file — the four occurrences are these two rendered at the base class plus its subclasses. No other file needs changing, and the issue overstated the spread.

The pristine-main build's actual warning log has exactly 4 cross-reference 'Type' warnings, and none of them come from pcapkit/foundation/traceflow/traceflow.py:

docs/source/pcapkit/foundation/engines/engine.rst:39:            WARNING: ... cross-reference 'Type': [same 5 classes]
docs/source/pcapkit/foundation/reassembly/reassembly.rst:32:     WARNING: ... cross-reference 'Type': [same 5 classes]
docs/source/pcapkit/foundation/reassembly/reassembly.rst:42:     WARNING: ... cross-reference 'Type': [same 5 classes]
docs/source/pcapkit/foundation/traceflow/traceflow.rst:39:       WARNING: ... cross-reference 'Type': [same 5 classes]

All four are hand-written :type: fields on manually-authored .. property:: directives, in three different .rst files:

  • docs/source/pcapkit/foundation/engines/engine.rst:40 — :type: dict[str, ModuleDescriptor[EngineBase] | Type[EngineBase]]
  • docs/source/pcapkit/foundation/reassembly/reassembly.rst:33 — :type: Type[Protocol]
  • docs/source/pcapkit/foundation/reassembly/reassembly.rst:43 — :type: dict[str, ModuleDescriptor[ReassemblyBase] | Type[ReassemblyBase]]
  • docs/source/pcapkit/foundation/traceflow/traceflow.rst:40 — :type: dict[str, ModuleDescriptor[TraceFlowBase] | Type[TraceFlowBase]]

None of these is Type[Dumper], and none is generated by autodoc rendering traceflow.py's two #: comments "at the base class plus its subclasses" — they document four different properties (EngineBase.registry, ReassemblyBase.protocol, ReassemblyBase.registry, TraceFlowBase.registry) via type text typed directly into the .rst files, entirely independent of Dumper or of this PR's two-line diff. This lines up with PR #694's own body, which names these same four locations (at engine.rst:39, reassembly.rst:32/:42, traceflow.rst:39 — the 1-line offset from what I found is just an unrelated line inserted in each file since #694 was written).

I confirmed the practical consequence directly: the "PR fix alone" build (claim 1-2) has the exact same 4 'Type' warnings as pristine main — unchanged. So after this PR merges, issue #709's original statement — "worth sweeping the other four pre-existing bare-Type sites in the same change, since they are the same defect" — is still true. The PR's "no other file needs changing" is not correct as a factual claim about the current warning set; three other files (engine.rst, reassembly.rst, and — separately from the two lines this PR touches — traceflow.rst's own hand-written property doc) carry the same defect and remain unaddressed.

To be clear about scope: the PR never claimed to fix those four; it's narrowly and correctly scoped to the _foutio/__output__ defect from #709's title. My objection is specifically to the added "correction to #709's own text" paragraph, which asserts something the build disproves and will misdirect anyone reading the git history into thinking the four pre-existing warnings were investigated and traced to this file.

8. No test — agree, and there's a direct precedent

Agree no test is warranted, and there's a better answer than my own read of the tradeoff: tests/project/test_documentation_claims.py's module docstring states the project's own established policy on this exact question, verbatim:

Deliberately not tested here either: whether a Sphinx cross-reference resolves. That is a property of the built inventory, not of the source, and the honest check for it is a nitpicky sphinx-build... A unit test that re-implemented Sphinx's resolution rules would pass while the real build failed, which is worse than not testing it. The measured counts are recorded in the pull request instead.

That's exactly what this PR does. I would not ask for a test here.

Summary

# Claim Verdict
1-2 before/after warning diff Confirmed — but the fix alone changes 0 warnings against real main today (both sites are latent)
3 _foutio reproduction Confirmed, exact warning text reproduced
4 ~typing.Type resolves, doesn't just silence Confirmed, and the pre-fix state is worse than described (wrong link, not just a warning)
5 8-file convention Confirmed, all 8 verified
6 scope grep = 0 Confirmed true; no live Protocol/Dumper/Schema collisions found either
7 "correction to #709" (4 occurrences = these 2 sites elsewhere) Contradicted — the 4 warnings come from 3 other .rst files, unaffected by this PR
8 no test needed Agree, with direct project precedent

The code diff itself is correct, minimal, and verified to do more good than described (claim 4). The change I'm asking for is to the prose: drop or correct the "correction to #709's own text" paragraph in the commit message/PR body before merge, since it's a factual claim the build disproves and it will otherwise sit in permanent git history misdescribing which files still need the same fix.

)

- `TraceFlow._foutio`'s `#:` comment named a bare `Type[Dumper]`, which
  Napoleon resolves against every class named `Type` in the project (five
  of them), so Sphinx warns `more than one target found for cross-reference
  'Type'` as soon as anything documents that attribute.
- `TraceFlow.__output__` carried the identical bare `Type[Dumper]` in its
  own `#:` comment, the same latent defect even though nothing currently
  triggers it.
- Qualified both as `~typing.Type[Dumper]`, matching the convention already
  used for this exact ambiguity in `pcapkit/protocols/__init__.py` and seven
  sibling files (`frame.py`, `sctp.py`, `link.py`, `tcp.py`, `internet.py`,
  `udp.py`, `pcapng.py`).
- Verified by temporarily mirroring the `autoattribute:: _foutio` directive
  a pending docs change adds: the bare form reproduces the warning, the
  qualified form clears it, confirmed against the rendered `typing.Type`
  link in the built HTML.

Build: `sphinx-build -b html` succeeds with no new warnings of any kind.
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE — the standing ❌ NEEDS CHANGES from issuecomment-5797644911 is cleared. The false "only two sites / #709 overstated the spread" claim is gone, and the replacement ## Correction section retracts it plainly and accurately: all four cited .rst sites exist at exactly the line numbers given, on current origin/main (699f757f2), verified after #694's merge — engine.rst:40, reassembly.rst:33 and :43, traceflow.rst:40, none of them shifted. Code is unchanged at f9edcd3f3 (1 file, +2/−2, lines 146 and 406), Part of #709 is correct, and the earlier review's open intersphinx question is now settled as a non-issue (Sphinx emits that warning only from the local domain inventory; intersphinx matches targets by exact key and cannot produce it). Two non-blocking notes in the write-up, one of which is good news: #694 has merged, so autoattribute:: _foutio is now live on main and this PR removes a real warning rather than a latent one — the body still describes #694 as open. Detailed write-up follows.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Detailed re-review

Re-reviewed on Opus, read-only in a git worktree, against origin/main at 699f757f2. No commits, no pushes, no edits — git status --porcelain is empty. The earlier round was issuecomment-5797644911 (verdict) and issuecomment-5797672785 (write-up).

I did not rebuild the docs. Per the re-review brief I am relying on the earlier round's measurement — 53 warnings pristine → 54 with the bare form under an autoattribute:: _foutio directive → 53 qualified, byte-identical to pristine — rather than repeating it. Everything else below is evidence I obtained myself.

1. Is the corrected body accurate, and is the retraction plain? — Yes

The false claim is gone. Searched the current body for every form of it: no "overstated", no "only two source sites", no "no other file needs changing". What replaced it is an H2 section, ## Correction: an earlier version of this description was wrong, and #709 was right, which quotes the retracted claim, says "That was wrong" in bold, and states that #709's original claim "was correct". The heading itself carries the words Correction, wrong, and #709 was right, so it cannot be skimmed past — that is a plain retraction, not a buried one.

I checked each factual assertion the new body makes, not just the retracted one:

body claim my verdict
four .rst sites at engine.rst:40, reassembly.rst:33, reassembly.rst:43, traceflow.rst:40 Confirmed on origin/main 699f757f2, verbatim, all four
"Those four warnings still fire unchanged after this merges" Confirmed — the diff touches only pcapkit/foundation/traceflow/traceflow.py; not one of the four files is in it
pcapkit/const/l2tp/type.py:18's class Type(IntEnum) Confirmed verbatim at line 18
commit message's "five of them" Confirmed — exactly 5 classes named Type under pcapkit/, and they are precisely the 5 the warning text lists (const/l2tp/type.py:18, protocols/application/ftp.py:40, protocols/application/httpv1.py:75, protocols/data/link/arp.py:34, vendor/l2tp/type.py:30)
the 8-file ~typing.Type convention Confirmed, all 8 read directly at the cited line numbers — see the nit in §6 below
grep -rn '#:.*Type\[' pcapkit/ --include='*.py' | grep -vc '~typing\.Type' → 0 Confirmed 0 on the PR tree
traceflow.rst:30 already uses the fully-qualified typing.Type[pcapkit.protocols.protocol.ProtocolBase] Confirmed verbatim at line 30 on current main

No remaining false claim. The body is now more accurate than the average PR body in this repo.

2. Do the four .rst line numbers still hold after #694? — Yes, all four, unshifted

This was the thing most likely to have broken, and it did not. Checked against origin/main 699f757f2, not against the body:

docs/source/pcapkit/foundation/engines/engine.rst:40          :type: dict[str, ModuleDescriptor[EngineBase] | Type[EngineBase]]
docs/source/pcapkit/foundation/reassembly/reassembly.rst:33   :type: Type[Protocol]
docs/source/pcapkit/foundation/reassembly/reassembly.rst:43   :type: dict[str, ModuleDescriptor[ReassemblyBase] | Type[ReassemblyBase]]
docs/source/pcapkit/foundation/traceflow/traceflow.rst:40     :type: dict[str, ModuleDescriptor[TraceFlowBase] | Type[TraceFlowBase]]

Identical to the same grep at e86d6b4f3 (the earlier review's base), so nothing moved across the three commits main took today. That is slightly lucky rather than obvious: #694 did edit two of these three files — engine.rst +2 and traceflow.rst +13 — but every added line landed below line 40 in both, and #694 did not touch reassembly.rst at all. So the body's line numbers are correct as written and need no update.

(For anyone comparing against the earlier write-up: it reported these as :39/:32/:42/:39. That is not a discrepancy — those are the line numbers Sphinx reports, which point at the .. property:: directive one line above the :type: field. The :type: lines themselves are :40/:33/:43/:40, which is what the body cites and what I grepped.)

3. Part of #709 vs Closes #709 — Part of is correct; the stated reason has gone stale

Part of is right. The four .rst sites are untouched by this diff and still carry the defect, so #709 is not closed by this merge. Changing Closes → Part of was the correct call.

But the body's justification for deferring them no longer holds:

The four .rst sites live under docs/, which open PR #694 owns, so fixing them here would collide. They are left for a follow-up once #694 lands.

#694 has landed — 81c63a365, on main as of today. So "open PR #694 owns" is no longer true and the collision risk it names is gone. The follow-up is unblocked as of now.

On whether the body should reference that follow-up: not yet, and it can't. git ls-remote --heads origin shows exactly one 709 branch — fix/709-ambiguous-type-xref, this PR. fix/709-rst-type-xrefs exists only locally; it is unpushed and has no PR, so there is no number or URL to cite. Once it is raised, a one-line edit to #712's body pointing at it would be worth having, but it is not a merge blocker for #712 and #712 should not wait on it.

4. Code unchanged? — Confirmed, byte for byte

Head is f9edcd3f364fcaa7df969213149457dd4faade19, one commit, as the brief states. git diff --stat origin/main...f9edcd3f3:

 pcapkit/foundation/traceflow/traceflow.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Both hunks are exactly what the earlier round verified — the #: prose at line 146 (__output__) and line 406 (_foutio), each gaining the ~typing. qualifier and nothing else:

-    #: DefaultDict[str, tuple[ModuleDescriptor[Dumper] | Type[Dumper], str | None]]:
+    #: DefaultDict[str, tuple[ModuleDescriptor[Dumper] | ~typing.Type[Dumper], str | None]]:
...
-        #: Type[Dumper]: Dumper class.
+        #: ~typing.Type[Dumper]: Dumper class.

No code, no behaviour, no test surface. The commit message is also unchanged — and worth noting, it never contained the retracted claim; that paragraph only ever lived in the PR body, so nothing false is heading into permanent git history from the message side.

5. Does typing.Type collide with intersphinx-indexed names (e.g. dictdumper)? — Settled: no, and it cannot

The earlier round flagged this and left it open. It is answerable from the Sphinx source without a build, so here is the answer.

First, the concern is real in setup, not hypothetical — dictdumper is in intersphinx_mapping (docs/source/conf.py:86), alongside chardet, dpkt, scapy, cryptography, requests and bs4. So if this failure mode existed, this project would be exposed to it.

It does not exist, for two independent reasons:

  1. Only the local domain can emit this warning. more than one target found for cross-reference %r appears at exactly one code site in Sphinx — sphinx/domains/python/__init__.py:974. It fires on len(matches) > 1 where matches comes from PythonDomain.find_obj, and find_obj reads only self.objects, the local Python domain inventory (lines 885-897: fullname in self.objects, f'{modname}.{name}' in self.objects, name in self.objects). Intersphinx objects are never in self.objects — they are resolved later, by the missing-reference handler, only after the domain has failed. So the 5-way Type ambiguity is definitionally a collision among local pcapkit classes, and typing.Type is not a local object, so it cannot participate in one.
  2. Intersphinx matches by exact key and never reports ambiguity. sphinx/ext/intersphinx/_resolve.py:94 is if target in inventory[objtype]: — a case-sensitive exact dict lookup, with the only fuzziness being a case-insensitive fallback restricted to std:label/std:term. There is no endswith-style search for py: objects. For dictdumper to shadow typing.Type, its inventory would need an entry keyed literally typing.Type; and even in that case intersphinx silently takes the first matching inventory rather than warning, so it still could not produce the warning docs: bare 'Type' in TraceFlow._foutio's autodoc comment is ambiguous across five classes, adding a Sphinx cross-reference warning #709 is about.

Empirically consistent too: the earlier round's build had all seven third-party inventories loaded and rendered _foutio as <a class="reference external" href="https://docs.python.org/3/library/typing.html#typing.Type">, with the warning count back to pristine. So the qualified form resolves to CPython's typing.Type with the full mapping in play.

I also checked the one local way this could have gone wrong: whether pcapkit ships a module named typing containing a class Type, which would make typing.Type fuzzy-matchable locally. It does not — no typing module anywhere under pcapkit/.

Consider this open question closed. ~typing.Type is safe against intersphinx shadowing by construction, not just by measurement.

6. One prose nit, non-blocking

The body says the eight convention files are "every one writing ModuleDescriptor[Protocol] | ~typing.Type[Protocol]". Seven of eight do. The eighth, pcapkit/protocols/__init__.py:72, is the single form with no union:

#: dict[str, ~typing.Type[Protocol]]: Protocol registry.

Same convention, same purpose, just not the same construction — "every one" is loose by one file. Cosmetic; I would not hold the PR for it. (The earlier write-up noted this too, in its §5.)

The one thing that has materially changed since the last review — and it favours this PR

autoattribute:: _foutio is now on main, at docs/source/pcapkit/foundation/traceflow/traceflow.rst:78, delivered by #694 (81c63a365). It was absent from that file at e86d6b4f3 (grep count 0). Consequences:

I am deliberately not blocking on this. It is descriptive lag created by a merge that happened after the body was written, not a false claim; it misdescribes nothing about the change itself, misdirects nobody about which files still need fixing (all four are correctly named), and errs by understating the PR's value. The commit message — the part that lands in history — says only "a pending docs change adds", which was true at authoring and is harmless now. Worth a one-line touch-up if the body is being edited anyway; not worth a round trip on its own.

Summary

# Question Verdict
1 Body accurate; false claim gone; retraction plain? Yes — no trace of the "only two sites" claim; retraction is an H2 that names itself wrong; all 8 other factual claims independently verified
2 Four .rst line numbers still hold post-#694? Yes, all four, unshifted — #694's edits to engine.rst/traceflow.rst all landed below line 40, reassembly.rst untouched
3 Part of #709 correct? Reference the follow-up? Part of is correct. Follow-up branch fix/709-rst-type-xrefs is unpushed with no PR, so nothing to reference yet. Stated reason for deferring is stale (#694 merged) — non-blocking
4 Code unchanged at f9edcd3f3? Confirmed — 1 file, +2/−2, lines 146 and 406, exactly as previously verified. Relying on the prior warning-count measurement, not repeating it
5 typing.Type vs intersphinx (dictdumper)? Settled — cannot collide. The warning is emitted only from the local domain inventory; intersphinx matches by exact key and never reports ambiguity
6 8-file convention claim Confirmed; "every one writing ModuleDescriptor[…] | ~typing.Type[…]" is loose for protocols/__init__.py:72. Cosmetic

The prose defect that caused the block has been retracted, and the retraction is itself correct — I verified every site it cites rather than taking its word. The code was never in question and has not moved. Standing NEEDS CHANGES cleared; good to merge on review grounds, leaving CI as the remaining gate.

@JarryShaw
JarryShaw merged commit 64a1c0a into main Sep 23, 2026
26 checks passed
@JarryShaw
JarryShaw deleted the fix/709-ambiguous-type-xref branch September 23, 2026 21:09
JarryShaw added a commit that referenced this pull request Sep 23, 2026
…#714)

Sphinx defines several classes named Type across pcapkit (including
pcapkit.const.l2tp.type.Type), so a bare `Type` in a hand-written
:type: field is ambiguous and resolves silently to the wrong one --
confirmed by a docs build showing engine.rst's `Type[EngineBase]`
linking to the L2TP enum instead of typing.Type.

Qualify the four remaining bare-Type :type: fields in the docs/source
tree to `typing.Type[...]`, matching the fully-qualified form already
used at traceflow.rst:30:

- docs/source/pcapkit/foundation/engines/engine.rst:40
- docs/source/pcapkit/foundation/reassembly/reassembly.rst:33,43
- docs/source/pcapkit/foundation/traceflow/traceflow.rst:40

Two more bare-Type sites remain in pcapkit/foundation/traceflow/
traceflow.py's `#:` comments (lines 146 and 406); those belong to the
still-open #712 and are left untouched here.

Part of #709.

Verified with a full `make docs` warning diff: the four
"more than one target found for cross-reference 'Type'" warnings drop
to zero with no new warnings, and the built HTML now links `Type` to
typing.Type instead of the L2TP enum.
JarryShaw added a commit that referenced this pull request Sep 23, 2026
A third cross-review found the same failure mode again: the last pass
replaced vague prose with false prose. This round deletes rather than
corrects.

- #702 entry: dropped the attribution of the seven guard raise lines to
  specific PRs. "the four mh.* guards came from #632" was false and
  self-contradicts the file's own #623 entry three sections earlier
  ("The range guard above it is untouched" -- a05f461's own commit
  message says the same). The entry does not need to say which PR added
  which guard; it only needs to say the lines were already covered.
- #709 entry: dropped the `TraceFlowBase.__dict__['__annotations__']`
  citation. Measured: that lookup raises KeyError, not "is empty" --
  `TraceFlowBase.__annotations__` (the resolved, non-dict-shortcut form)
  is `{'__cached__': ...}`, non-empty. The conclusion (`__output__` is
  unannotated) holds, but the cited evidence does not, so it's dropped
  rather than replaced with a corrected measurement.
- #709 entry: rewritten to the present tense of a merged world. #709
  closed once #712 landed (the two traceflow.py sites this entry
  describes); the four docs/*.rst sites are fixed separately by #714,
  also merged. No more "does not close" / "stays open" hedging.

Refs #657
@JarryShaw JarryShaw removed the bug label Sep 23, 2026
JarryShaw added a commit that referenced this pull request Sep 24, 2026
…tale tense

Round 5: fixed the two defects the fourth cross-review found at head
93a4126 (NEEDS CHANGES).

- `:2389` -- "This pair of sites is what closes #709" was false: #712
  and #714 both state in their own bodies that they are `Part of #709`,
  not a close, and #709's closed timeline event carries `commit_id:
  null` (a manual close). Made non-causal: "This pair of sites is part
  of #709."
- `:2325` -- "#709 tracks it" was present tense describing an issue
  that is now closed, contradicting the #709 entry itself. Changed to
  past tense: "#709 tracked it."

Regenerated CHANGELOG.md from the edited entry.

| check | result |
|---|---|
| `changelog_md.py --check` | exit 0 |
| `pytest tests/project/test_changelog_md.py -q` | 47 passed, 37 subtests |
JarryShaw added a commit that referenced this pull request Sep 26, 2026
A third cross-review found the same failure mode again: the last pass
replaced vague prose with false prose. This round deletes rather than
corrects.

- #702 entry: dropped the attribution of the seven guard raise lines to
  specific PRs. "the four mh.* guards came from #632" was false and
  self-contradicts the file's own #623 entry three sections earlier
  ("The range guard above it is untouched" -- a05f461's own commit
  message says the same). The entry does not need to say which PR added
  which guard; it only needs to say the lines were already covered.
- #709 entry: dropped the `TraceFlowBase.__dict__['__annotations__']`
  citation. Measured: that lookup raises KeyError, not "is empty" --
  `TraceFlowBase.__annotations__` (the resolved, non-dict-shortcut form)
  is `{'__cached__': ...}`, non-empty. The conclusion (`__output__` is
  unannotated) holds, but the cited evidence does not, so it's dropped
  rather than replaced with a corrected measurement.
- #709 entry: rewritten to the present tense of a merged world. #709
  closed once #712 landed (the two traceflow.py sites this entry
  describes); the four docs/*.rst sites are fixed separately by #714,
  also merged. No more "does not close" / "stays open" hedging.

Refs #657
JarryShaw added a commit that referenced this pull request Sep 26, 2026
…tale tense

Round 5: fixed the two defects the fourth cross-review found at head
93a4126 (NEEDS CHANGES).

- `:2389` -- "This pair of sites is what closes #709" was false: #712
  and #714 both state in their own bodies that they are `Part of #709`,
  not a close, and #709's closed timeline event carries `commit_id:
  null` (a manual close). Made non-causal: "This pair of sites is part
  of #709."
- `:2325` -- "#709 tracks it" was present tense describing an issue
  that is now closed, contradicting the #709 entry itself. Changed to
  past tense: "#709 tracked it."

Regenerated CHANGELOG.md from the edited entry.

| check | result |
|---|---|
| `changelog_md.py --check` | exit 0 |
| `pytest tests/project/test_changelog_md.py -q` | 47 passed, 37 subtests |
JarryShaw added a commit that referenced this pull request Sep 26, 2026
A third cross-review found the same failure mode again: the last pass
replaced vague prose with false prose. This round deletes rather than
corrects.

- #702 entry: dropped the attribution of the seven guard raise lines to
  specific PRs. "the four mh.* guards came from #632" was false and
  self-contradicts the file's own #623 entry three sections earlier
  ("The range guard above it is untouched" -- a05f461's own commit
  message says the same). The entry does not need to say which PR added
  which guard; it only needs to say the lines were already covered.
- #709 entry: dropped the `TraceFlowBase.__dict__['__annotations__']`
  citation. Measured: that lookup raises KeyError, not "is empty" --
  `TraceFlowBase.__annotations__` (the resolved, non-dict-shortcut form)
  is `{'__cached__': ...}`, non-empty. The conclusion (`__output__` is
  unannotated) holds, but the cited evidence does not, so it's dropped
  rather than replaced with a corrected measurement.
- #709 entry: rewritten to the present tense of a merged world. #709
  closed once #712 landed (the two traceflow.py sites this entry
  describes); the four docs/*.rst sites are fixed separately by #714,
  also merged. No more "does not close" / "stays open" hedging.

Refs #657
JarryShaw added a commit that referenced this pull request Sep 26, 2026
…tale tense

Round 5: fixed the two defects the fourth cross-review found at head
93a4126 (NEEDS CHANGES).

- `:2389` -- "This pair of sites is what closes #709" was false: #712
  and #714 both state in their own bodies that they are `Part of #709`,
  not a close, and #709's closed timeline event carries `commit_id:
  null` (a manual close). Made non-causal: "This pair of sites is part
  of #709."
- `:2325` -- "#709 tracks it" was present tense describing an issue
  that is now closed, contradicting the #709 entry itself. Changed to
  past tense: "#709 tracked it."

Regenerated CHANGELOG.md from the edited entry.

| check | result |
|---|---|
| `changelog_md.py --check` | exit 0 |
| `pytest tests/project/test_changelog_md.py -q` | 47 passed, 37 subtests |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Pull requests that change documentation only (docs: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs: bare 'Type' in TraceFlow._foutio's autodoc comment is ambiguous across five classes, adding a Sphinx cross-reference warning

1 participant