docs: fix 45 places where the documentation contradicts the code - #413
Merged
Merged
Conversation
The house convention copies prose out of a module docstring into its `.rst` rather than pulling it in with `automodule`, which means a docstring edit does not follow into the page. A lot of source changed recently, so a lot of pages had drifted. This is the sweep for contradictions -- a page naming a parameter that does not exist, a default that changed, a limitation since fixed -- not for thinness. The one that was actively breaking the build: `engines/index.rst` declared `.. _libpcap:` **twice**, the only duplicate explicit target in the tree, so all three ```libpcap`_`` links were dead and the build emitted four errors. The C library reference now uses the page's own ``:manpage:`libpcap(3)``` idiom and the PyPI target is kept. Documentation for work that had none: `Engine.unsupported_reason` (new in #396, undocumented on `engines/engine.rst` while two other pages already cross-referenced it), the whole `_pcap_backend` module including `Probe` -- noted as a Mapping rather than a tuple, since it is an `Info` subclass now -- and `PyPCAPFile.PYTHON_CEILING`. The Scapy section still described the pre-#409 `scapy.sendrecv` import and omitted the `CryptographyDeprecationWarning` note that its docstring gained. Examples that could not have worked as written, all measured: `extract(strict=True)` raises `TypeError` -- the argument is `reasm_strict`; `python -m pypcapkit` has no such module, only the PyPI *name* is `pypcapkit` and the runnable one is `pcapkit`; three CLI transcripts relied on extension autocorrect that is gated behind `-a`, so they raised `FileNotFound`; and "export to a JSON file with no format specified" actually writes a *tree* dump into `out.json`, because `format=None` defaults to `'tree'`. The RFC 815 walkthrough on `reassembly/tcp.rst` had drifted from the implementation in six separate ways -- an exclusive `last` where the code is inclusive (`first + len - 1`), `ISN <- DSN` where a SYN spends a sequence number (`PSN = DSN + 1 if SYN`), an unconditional hole update the code guards with `if info.len > 0`, comparison operators that do not match, a `more_fragments` flag TCP does not have, and a four-tuple BUFID in the wrong order. And `reassembly/ip/ipv6.rst` still documented the reassembly key as the **flow label**, which is exactly the defect fixed in `3642dcaa9` -- the label is optional and routinely zero, so keying on it collapsed distinct datagrams. Nine docstrings were fixed rather than their pages, in the cases where the page was right and the docstring named something that does not exist -- `pcapkit.dumper.*`, `pcapkit.traceflow`, `pcapkit.protocols.null`, `Extrator`, `tractflow`, `TOS*` for `ToS*`, and `ipv6_opts.py` declaring itself as `hopopt`. Confined to that case deliberately, so the sweep does not leave a page and its docstring newly inconsistent by its own hand. Verified against a pristine baseline built from `git archive HEAD` in its own venv, after discarding a first attempt that had overlapped with in-flight edits: **278 warning lines and 4 errors before, 274 and 1 after** -- zero new, four eliminated, all four the `libpcap` family. The surviving error is pre-existing and untouched. All 2671 autodoc targets across all 128 pages resolve, and the rendered HTML was checked rather than assumed: `unsupported_reason`, `Probe`, `PCAP_CT.backend`, `PYTHON_CEILING` and `Backend Detection` all appear, and `index.html` now emits four working libpcap(3) links where all three were broken.
…ethods
Following the maintainer's decision that the octet tables exist to present the
header structure as the RFCs define it, not to mirror the library's API -- but that
the field names in them should still match what the code actually exposes.
`` `tcp.opt` `` -> `` `tcp.options` `` (`data/transport/tcp.py:100`,
`options: 'OrderedMultiDict[OptionNumber, Option]'`; nothing named `opt` exists) and
`` `l2tp.ver` `` -> `` `l2tp.version` `` (`data/link/l2tp.py:44`, `version: 'int'`).
The `tcp.options` cell needed re-padding afterwards, since the longer name pushed
the description out of its column.
`tcp.flags.ns` is a different case and stays. The bit *is* read off the wire --
`schema/transport/tcp.py:58` declares `ns: int` in the flags bitfield -- but the
data model and the read site have been commented out in lockstep since `2e39aeb99`
("minor revision for TCP on typings", 2023-06-29):
data/transport/tcp.py:52 #ns: 'bool'
protocols/transport/tcp.py:447 #ns=bool(schema.offset['ns']),
Left commented, deliberately: :rfc:`3540` was reclassified as Historic, and
reviving a field nothing consumes adds surface for no gain. The table row is kept
because the bit belongs to the header as the RFC defines it -- but it now carries a
footnote saying the library does not surface it, so a reader does not go looking for
`tcp.flags.ns` and find nothing.
`mh.rst` listed `_read_opt_pad` and `_make_opt_pad` **twice each**, which is where
two of the tree's duplicate-object warnings came from. Verified there is no `padn`
sibling that the second line was meant to be -- Pad1 and PadN deliberately share
one handler -- so they were simply stray copies.
There was a problem hiding this comment.
🟡 Changes recommended
The IPv4/IPv6 reassembly documentation examples still disagree with the actual construction of reassembly packet inputs (notably header/payload sources and payload types), which undermines the PR’s stated goal of eliminating doc/code contradictions.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR performs a repo-wide documentation/docstring sweep to eliminate contradictions with the current codebase, addressing broken Sphinx references, incorrect API/CLI usage examples, and drift between .rst content and copied docstrings.
Changes:
- Fixes incorrect module/class references and Sphinx link targets (including previously broken references like
ToSPrecedencecasing and duplicate autodoc entries). - Updates user-facing docs to match current public API/CLI behavior (e.g.,
reasm_strict,python -m pcapkit, engine notes). - Aligns various package/module docstrings under
pcapkit/with their canonical module paths and current semantics.
File summaries
| File | Description |
|---|---|
| pcapkit/vendor/ipv4/init.py | Fixes enum class references (ToS*) to match actual exported names. |
| pcapkit/vendor/esp/init.py | Updates module docstring wording for ESP vendor crawlers and IKEv2 registry explanation. |
| pcapkit/protocols/misc/null.py | Corrects module/class references in the module docstring. |
| pcapkit/protocols/internet/ipv6_opts.py | Fixes incorrect .. module:: directive to match actual module. |
| pcapkit/foundation/traceflow/init.py | Corrects module path referenced in docstring. |
| pcapkit/foundation/init.py | Fixes typos and incorrect module references in package docstring. |
| pcapkit/dumpkit/pcap.py | Fixes .. module:: directive to the correct dumpkit path. |
| pcapkit/dumpkit/null.py | Fixes .. module:: directive to the correct dumpkit path. |
| pcapkit/const/ipv4/init.py | Fixes enum class references (ToS*) to match actual exported names. |
| docs/source/pcapkit/vendor/index.rst | Updates the vendor crawler overview text. |
| docs/source/pcapkit/vendor/http.rst | Corrects const module references for generated HTTP enumerations and adjusts wording. |
| docs/source/pcapkit/utilities/functools.rst | Fixes a broken schema type reference (schema, not schmea). |
| docs/source/pcapkit/protocols/transport/tcp.rst | Updates field-name references (tcp.options) and adds an NS-bit footnote. |
| docs/source/pcapkit/protocols/misc/null.rst | Corrects module/class references for NoPayload. |
| docs/source/pcapkit/protocols/link/l2tp.rst | Updates octet-table field name (l2tp.version). |
| docs/source/pcapkit/protocols/internet/mh.rst | Removes duplicate automethod:: entries that caused duplicate renders. |
| docs/source/pcapkit/protocols/index.rst | Updates IPsec family diagram and adds missing ESP link target. |
| docs/source/pcapkit/interface/core.rst | Updates engine-selection docs to include pcap-ct and its link target. |
| docs/source/pcapkit/foundation/traceflow/index.rst | Fixes module path text and corrects the ext.html section anchor. |
| docs/source/pcapkit/foundation/reassembly/tcp.rst | Updates TCP reassembly terminology/algorithm to match current implementation details. |
| docs/source/pcapkit/foundation/reassembly/reassembly.rst | Fixes _IT type documentation to match current typing. |
| docs/source/pcapkit/foundation/reassembly/ip/ipv6.rst | Updates IPv6 reassembly terminology and buffer key documentation. |
| docs/source/pcapkit/foundation/reassembly/ip/ipv4.rst | Updates IPv4 reassembly terminology to match current attribute names/types. |
| docs/source/pcapkit/foundation/reassembly/ip/ip.rst | Fixes _AT union type to include IPv6 addresses. |
| docs/source/pcapkit/foundation/reassembly/index.rst | Fixes Mermaid click targets for reassembly pages/classes. |
| docs/source/pcapkit/foundation/index.rst | Fixes incorrect class/module references for foundation components. |
| docs/source/pcapkit/foundation/extraction.rst | Updates engine support docs to include pcap-ct and correct links. |
| docs/source/pcapkit/foundation/engines/index.rst | Improves engine prerequisites table and corrects method target for unsupported_reason. |
| docs/source/pcapkit/foundation/engines/engine.rst | Adds unsupported_reason to documented Engine API surface. |
| docs/source/pcapkit/foundation/engines/3rdparty.rst | Adds the missing third-party engines page and expands autodoc coverage. |
| docs/source/pcapkit/dumpkit/pcap.rst | Fixes module directive to pcapkit.dumpkit.pcap. |
| docs/source/pcapkit/dumpkit/null.rst | Fixes module directive to pcapkit.dumpkit.null. |
| docs/source/pcapkit/dumpkit/index.rst | Fixes Mermaid class relationships and corrects a click target for NotImplementedIO. |
| docs/source/pcapkit/const/ipv4.rst | Fixes enum class references (ToS*) to match actual exported names. |
| docs/source/ext.rst | Adds ESP to the protocol table to match current protocol set. |
| docs/source/demo.rst | Fixes example API parameter name (reasm_strict) and corrects python -m pcapkit usage notes and CLI samples. |
Review details
- Files reviewed: 32/36 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both Copilot findings on #413 hold up. The IPv4 and IPv6 reassembly glossary snippets named attributes that do not exist: - `ipv4.header` is not an attribute at all (`hasattr(IPv4, 'header')` is False, and `header` is not a field of the IPv4 data model). `toolkit/pcap.py` uses `ipv4.packet.header` and `bytearray(ipv4.packet.payload)`. - The IPv6 snippet's `ipv6.header` / `ipv6.payload` are really `ipv6_info.fragment.header` / `bytearray(ipv6_info.fragment.payload)`. Both snippets also elided the `.info` hop and wrote `tuple(a, b, c, d)`, which is not how `tuple` is called. They now mirror `toolkit/pcap.py` name for name, with a lead-in saying which object is which. The second finding's other half turned out to be a library defect rather than a comment error: `ihl` and `header` on this path *do* include the Fragment header, because `ipv6.py:341` adds each extension header's length before the Fragment-header check breaks the loop. `dpkt` and `scapy` exclude it and report 40 where this reports 48 for the same packet, and RFC 8200 s4.5 says the Fragment header is absent from a reassembled packet. Filed as #415; the doc now states what the code does today and warns that the value is not comparable across engines, rather than asserting semantics that are about to change.
JarryShaw
added a commit
that referenced
this pull request
Sep 16, 2026
…workaround `conf.py` was normalising `rank: ' int'` at build time because the file holding the second instance was thought to be off-limits to this branch. It is not: #413 edits `pcapkit/protocols/internet/ipv6_opts.py`, and the defect is in `pcapkit/protocols/data/internet/ipv6_opts.py` -- a different file that no open PR touches. So it is fixed at source and `strip_annotation_whitespace` is gone, along with the warning it emitted on every build. A sweep of the package finds no third instance. The reason the trailing space matters is kept, moved onto `bind_type_checking_names` where it belongs: resolving the guarded names is what turns a malformed annotation from harmless into fatal, since these used to fail earlier with `NameError` (which Sphinx catches) and now reach 3.14's `annotationlib`, which raises `SyntaxError` (which Sphinx does not). A third such typo should stop the build rather than be papered over here. Clean build after: 91 warning/error lines, 0 signature failures, 0 stray-whitespace notices, 516 pages. Unit tier 649 passed, 5 skipped.
Owner
Author
|
@copilot resolve the merge conflicts on this branch. |
# Conflicts: # docs/source/pcapkit/interface/core.rst Co-authored-by: JarryShaw <15666417+JarryShaw@users.noreply.github.com>
Contributor
JarryShaw
added
the
docs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A sweep of the whole
docs/source/tree against the code it describes, plus thedocstring copies that had drifted from their originals. 45 contradictions, all
verified against the source before changing anything.
The ones that broke something visible
.. _libpcap:target meant docutils resolved none of its threereferences -- 4 Sphinx errors from one line.
extract(strict=True)does not exist; the parameter isreasm_strict.python -m pypcapkitis documented indemo.rst; there is no such module(the console script is
pcapkit).TOSPrecedenceis spelledToSPrecedenceinpcapkit.const.ipv4, so everyreference to it was a dead link.
reassembly/ip/ipv6.rststill documented the flow-label buffer key that3642dca removed.
automethod::lines inmh.rst(_read_opt_pad/_make_opt_pad), which autodoc rendered twice.Field names that had moved
tcp.opt->tcp.optionsandl2tp.ver->l2tp.versionin the octet tables.Those tables are transcribed from the RFCs to show header structure, so they do
not have to match the library's attribute names -- but where they name a field
the library exposes under a different name, they should use the library's. The
tcp.flags.nsrow stays, with a footnote saying the library does not surface it:the bit is historical (ECN nonce, removed by RFC 8311) and the table is a picture
of the header, not of the API.
The engines page
foundation/engines/3rdparty.rstwas missing entirely, so the module itdocuments had no page. Added following the convention the other protocol pages
use -- docstring copied, no
automodule-- per review feedback on #408.Docstrings, not just pages
Nine of the fixes are in
pcapkit/itself, where the.rstwas right and thedocstring it was copied from had drifted (or vice versa). Both sides now agree.
Sphinx builds with no new warnings; the four
libpcaperrors are gone.