From e24f17dc60c5cf7f01ba0d7a326b061a4a57a475 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 20 Sep 2026 01:45:51 -0400 Subject: [PATCH] docs(hopopt): cite RFC 8200 section 4.2 for the Opt Data Len sentence Closes #530. - `_hopopt_option_length` quoted "the length of the Option Data field of this option, in octets" and attributed it to RFC 8200 section 4.3. That sentence is the `Opt Data Len` definition from section 4.2; section 4.3 defines no `Opt Data Len` at all and its only length field is `Hdr Ext Len`, the whole header in 8-octet units. Now cites 4.2 for the quote and keeps 4.3 as the header this class implements, matching the two-part treatment #528 landed for the IPv6-Opts sibling. - `tests/test_docstring_contract.py` grows a fourth property: every verbatim RFC 8200 sentence this package quotes must be introduced by a citation naming the section that contains it. Keyed on the quote rather than the file, so it covers the next copy-paste of these paragraphs. - Drops the rotted `KNOWN_DEFECTS` entry for `pcapkit/vendor/ipx/packet.py`, whose `process` now takes and documents `data`. Unrelated to #530: #524 renamed the parameter and #535 added the rot guard four commits later, so `main` has been red on that subtest since the guard landed. No behavioural change: the docstring is prose, and `_hopopt_option_length` still returns `schema_len + 2`. Verified on .venv python 3.14.7 with PYTHONSAFEPATH=1 and pcapkit.__file__ asserted inside the worktree. The new check fails on unfixed hopopt.py (1 defect) and passes with the fix (0). Suite now fully green, exit code read from a file rather than a pipe: 66 passed, 449 subtests passed, EXIT=0. --- pcapkit/protocols/internet/hopopt.py | 20 ++- tests/test_docstring_contract.py | 198 +++++++++++++++++++++++++-- 2 files changed, 203 insertions(+), 15 deletions(-) diff --git a/pcapkit/protocols/internet/hopopt.py b/pcapkit/protocols/internet/hopopt.py index 5a3c0d1bb..4748a96f6 100644 --- a/pcapkit/protocols/internet/hopopt.py +++ b/pcapkit/protocols/internet/hopopt.py @@ -453,7 +453,7 @@ def _read_opt_type(self, kind: 'int') -> 'tuple[int, bool]': def _hopopt_option_length(schema_len: 'int') -> 'int': """Compute a HOPOPT option's whole-option length from its on-the-wire ``Opt Data Len``. - Per :rfc:`8200#section-4.3`, an option's ``Opt Data Len`` field + Per :rfc:`8200#section-4.2`, an option's ``Opt Data Len`` field (what each ``Schema_*Option.len`` here holds) counts *"the length of the Option Data field of this option, in octets"* -- i.e. it excludes the Option Type and Opt Data Len fields themselves, so the @@ -466,6 +466,24 @@ def _hopopt_option_length(schema_len: 'int') -> 'int': arithmetic only has to happen once. Do NOT drop the ``+ 2``: that is precisely the mismatch #398 fixed. + Section 4.2 is the citation because it is what defines the TLV option + format, and it is where the sentence quoted above actually appears. + This cited :rfc:`8200#section-4.3` until #530, which is a subtler + error than the one #517 fixed in the IPv6-Opts sibling: Section 4.3 + is not the wrong *header* -- it is the Hop-by-Hop Options header, + which is exactly what this class implements -- but it is the wrong + place for this arithmetic. It defines no ``Opt Data Len`` at all, + deferring the option encoding to Section 4.2 (*"one or more + TLV-encoded options, as described in Section 4.2"*), and its own only + length field is ``Hdr Ext Len``, *"the length of the Hop-by-Hop + Options header in 8-octet units, not including the first 8 octets"* -- + the whole header in 8-octet units, which is a different quantity from + one option's ``Opt Data Len`` in octets. So a reader who followed the + old citation found the right header and no such sentence. Section + 4.3 stays the right reference for the header itself and is still + cited as such below, at the ``Note:`` on the whole extension header + having to be a multiple of 8 octets. + Note that only the *stored-length* read-side call sites are collected here -- most ``_make_opt_*`` methods recompute the wire ``Opt Data Len`` from ``len(value)`` rather than reading a parsed diff --git a/tests/test_docstring_contract.py b/tests/test_docstring_contract.py index 62145f56a..68d1417c1 100644 --- a/tests/test_docstring_contract.py +++ b/tests/test_docstring_contract.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -"""Every ``Args:`` name and ``Raises:`` clause, checked against the code itself. +"""Every ``Args:`` name, ``Raises:`` clause, and quoted RFC sentence, checked. GitHub issue #501 fixed one wrong exception name and forty stale ``Args:`` labels by hand. Issue #519 then showed the class was not exhausted -- more @@ -16,9 +16,10 @@ docstring written tomorrow is checked tomorrow rather than being a snapshot of what was wrong in 2026. -Three properties are asserted, and the split between them is deliberate -- +Four properties are asserted, and the split between them is deliberate -- each is sound on its own, and the softer judgements are recorded rather than -asserted: +asserted. The first three derive their answer from the code; the fourth cannot, +and says so: :meth:`DocstringParameterTests.test_documented_parameters_exist` A documented name that is not a parameter *and* has no ``**kwargs`` to @@ -38,6 +39,13 @@ positive here would make the suite fail on a correct docstring, which is worse than missing one. +:meth:`DocstringCitationTests.test_quoted_rfc_sentences_cite_their_own_section` + A docstring that quotes a sentence from :rfc:`8200` verbatim and attributes + it to a section that does not contain it. Issues #517 and #530 were both + exactly this, in sibling files, and neither was catchable by anything here + before -- see :data:`RFC8200_QUOTED_SENTENCES` for what this can and cannot + prove. + Why a documented name may legitimately be absent from the signature, which is the trap this module has to avoid: this project documents keyword arguments consumed from ``**kwargs``, and does so correctly. @@ -93,11 +101,18 @@ ``data`` where the parameter was ``soup``. Rebasing this change onto ``c8fd97bcd`` picked up #511, which retired the HTML scrape and renamed that parameter to ``data`` as a side effect -- so the defect was gone, and this test -failed on exactly that subtest and nothing else. The entry was removed. Worth -knowing that the failure is reported through :meth:`~unittest.TestCase.subTest`, -and ``pytest-subtests`` is not a dependency here, so pytest prints the parent -test as ``PASSED`` while exiting non-zero: read the exit code, not the summary -line. +failed on exactly that subtest and nothing else. The entry was removed. + +It then happened a second time, to the sibling ``pcapkit/vendor/ipx/packet.py``, +and that entry's removal is in this change's diff. ``a8912b46f`` (#524) renamed +that ``process``'s parameter to ``data`` so its docstring became correct, and +``48c6f07f8`` (#535) added this list four commits later without re-running +against the newer base -- so ``main`` was red on that subtest from the moment +the guard itself landed. The guard was right both times; what failed was +re-running it after a rebase. Worth knowing that the failure is reported through +:meth:`~unittest.TestCase.subTest`, and ``pytest-subtests`` is not a dependency +here, so pytest prints the parent test as ``PASSED`` while exiting non-zero: +read the exit code, not the summary line. Everything here reads :file:`pcapkit/` source and imports nothing but the standard library, so it reads no capture under :file:`examples/captures/` and @@ -108,6 +123,7 @@ import ast import pathlib +import re import unittest from typing import TYPE_CHECKING, NamedTuple @@ -137,6 +153,47 @@ 'Warnings', 'Warns', 'Yield', 'Yields', }) +#: Sentences this package quotes verbatim from :rfc:`8200`, each mapped to the +#: section that actually contains it. Keys are lowercased and whitespace is +#: normalised before matching, because every one of these quotes is line-wrapped +#: across a docstring and the leading ``Length of`` is routinely lowercased to +#: fit the sentence it is embedded in. +#: +#: **What this proves and what it does not.** The section numbers here are not +#: derived from anything in this repository -- they were read out of +#: :rfc:`8200` itself, whose text is not vendored here and is not fetched by the +#: suite. So this table is a recorded reading, and +#: :meth:`DocstringCitationTests.test_quoted_rfc_sentences_cite_their_own_section` +#: does not verify the RFC. What it verifies is the *pairing*: that wherever one +#: of these sentences is quoted, the citation introducing it names the section +#: the sentence came from. That is the invariant #517 and #530 both broke, and +#: it is checked at every site rather than pinned at the two that are currently +#: wrong, so the next copy-paste of one of these paragraphs into a third module +#: is checked when it lands rather than being read past. +#: +#: Verified against https://www.rfc-editor.org/rfc/rfc8200.txt on 2026-09-20: +#: ``Opt Data Len`` is defined only in Section 4.2, which is also where Pad1 and +#: PadN live; Section 4.3 (Hop-by-Hop Options) and Section 4.6 (Destination +#: Options) each define a ``Hdr Ext Len`` in 8-octet units and defer TLV +#: encoding to Section 4.2 with the words quoted below; Section 4.4 (Routing) +#: defines the third ``Hdr Ext Len``. +#: +#: Values are *sets* of acceptable sections rather than one section, because a +#: sentence can honestly belong to more than one: the TLV deferral below is +#: worded identically in Section 4.3 and Section 4.6 (rfc8200.txt lines 722 and +#: 1268), so a docstring citing either is right and a checker demanding one +#: would fail a correct citation in the sibling header. +RFC8200_QUOTED_SENTENCES = { + 'the length of the option data field of this option, in octets': + frozenset({'4.2'}), + 'one or more tlv-encoded options, as described in section 4.2': + frozenset({'4.3', '4.6'}), + 'the length of the hop-by-hop options header in 8-octet units, ' + 'not including the first 8 octets': frozenset({'4.3'}), + 'the length of the routing header in 8-octet units, ' + 'not including the first 8 octets': frozenset({'4.4'}), +} + class Finding(NamedTuple): """One docstring defect, keyed so it survives the lines moving.""" @@ -172,12 +229,6 @@ class Known(NamedTuple): '_make_ipv4_options', 'option'), "documents 'option' where the parameter is 'options'; ipv4.py is " 'owned by another change'), - Known(Finding('pcapkit/vendor/ipx/packet.py', 'process', 'data'), - "documents 'data' where the parameter is 'soup'; pcapkit/vendor is " - 'generated-adjacent and owned elsewhere. Note the sibling ' - 'pcapkit/vendor/ipx/socket.py carried the identical defect and no ' - 'longer does, so that umbrella covers the file rather than the ' - 'directory -- see the module docstring'), Known(Finding('pcapkit/vendor/mh/binding_ack_flag.py', 'context', 'soup'), "documents 'soup: Parsed HTML source.' where the parameter is 'data' " "and holds CSV rows. Nothing was renamed here despite the " @@ -452,6 +503,67 @@ def unreachable_exceptions() -> 'list[Finding]': if not _reachable(node, exception)] +def docstrings() -> 'Iterator[tuple[str, str, str]]': + """Every docstring under :file:`pcapkit/`, with the name of what it documents. + + Wider than :func:`functions`, which yields only function docstrings, because + a quoted RFC sentence is just as likely to sit in a module or class docstring + as in a method's -- :class:`~pcapkit.protocols.schema.schema.Schema` + subclasses carry their wire-format prose at class level. Kept separate rather + than widening :func:`functions` because the ``Args:`` and ``Raises:`` checks + are about signatures and ``raise`` statements, neither of which a module or + class docstring has, so feeding them these nodes would be meaningless. + + """ + for path in sorted(PACKAGE.rglob('*.py')): + try: + tree = ast.parse(path.read_text(encoding='utf-8'), str(path)) + except (SyntaxError, UnicodeDecodeError): # pragma: no cover + continue + relative = path.relative_to(ROOT).as_posix() + for node in ast.walk(tree): + if not isinstance(node, (ast.Module, ast.ClassDef, ast.FunctionDef, + ast.AsyncFunctionDef)): + continue + doc = ast.get_docstring(node, clean=False) + if doc: + yield relative, getattr(node, 'name', ''), doc + + +def misattributed_quotes() -> 'list[Finding]': + """Quoted :rfc:`8200` sentences whose introducing citation names another section. + + "Introducing" is the nearest ``8200#section-`` citation *before* the quote in + the same docstring, which is how these paragraphs are actually written -- + ``Per :rfc:`8200#section-4.2`, an option's ``Opt Data Len`` field ... counts + *"the length of the Option Data field of this option, in octets"*``. Taking + the nearest preceding one rather than asking whether the right section is + cited *anywhere* in the docstring is what gives the check teeth: both #517 + and #530 cited the correct section elsewhere in the same paragraph while + attributing the quote to the wrong one, so an "is it mentioned" test would + have passed on both defects. + + A docstring that quotes one of these sentences with no preceding citation at + all is reported too -- an unattributed verbatim quote is the same defect with + the citation missing rather than wrong. + + """ + defects = [] # type: list[Finding] + for module, name, doc in docstrings(): + flat = ' '.join(doc.split()).lower() + for sentence, sections in RFC8200_QUOTED_SENTENCES.items(): + start = flat.find(sentence) + if start < 0: + continue + cited = re.findall(r'8200#section-([0-9]+(?:\.[0-9]+)*)', flat[:start]) + if cited and cited[-1] in sections: + continue + defects.append(Finding(module, name, '%s attributed to %s, not %s' + % (sentence[:40], cited[-1] if cited else 'nothing', + '/'.join(sorted(sections))))) + return defects + + class DocstringParameterTests(unittest.TestCase): """``Args:`` entries against the real signatures.""" @@ -568,5 +680,63 @@ def test_commented_out_raise_leaves_no_clause_behind(self) -> 'None': self.assertEqual(stale, [], '\n'.join(['stale clause(s):'] + stale)) +class DocstringCitationTests(unittest.TestCase): + """Verbatim RFC quotes against the section they are attributed to.""" + + def test_quoted_rfc_sentences_cite_their_own_section(self) -> 'None': + """No docstring attributes an :rfc:`8200` sentence to the wrong section. + + This is the class of defect #517 and #530 were both instances of, in + sibling files, six weeks apart -- and the reason it needed a check rather + than a second careful reading is that the two look nothing alike at the + point of the mistake. #517's ``ipv6_opts.py`` cited Section 4.3 for a + Section 4.2 sentence while implementing the Section 4.6 header, so the + cited section was unrelated to the file. #530's ``hopopt.py`` cited + Section 4.3 for the same Section 4.2 sentence while implementing the + Section 4.3 header, so the cited section was the *right* one for the + class and the wrong one for the quote. A reviewer who learned to look + for "cites a header it does not implement" catches the first and reads + straight past the second. + + What makes the assertion safe is that it is keyed on the quote, not on + the file: the only docstrings it can fail are ones that reproduce a + sentence from :rfc:`8200` word for word, where there is a fact of the + matter about which section that sentence is in and no judgement to make. + Prose that paraphrases the RFC, or cites a section without quoting it, is + not matched and cannot be failed -- see :data:`RFC8200_QUOTED_SENTENCES` + for why the section numbers are a recorded reading rather than a + derivation, and what that costs. + + """ + unexpected = misattributed_quotes() + self.assertEqual(unexpected, [], '\n'.join( + ['%d quoted RFC sentence(s) attributed to the wrong section:' + % len(unexpected)] + + [' %s %s(): %s' % finding for finding in unexpected])) + + def test_the_quoted_sentences_are_still_quoted_somewhere(self) -> 'None': + """Every :data:`RFC8200_QUOTED_SENTENCES` key still appears in the package. + + Without this, the table above decays into a lie the moment one of these + paragraphs is reworded: the entry stops matching anything, the pairing + check silently covers one site fewer, and nothing fails. The same + rot-guard reasoning as + :meth:`DocstringParameterTests.test_known_defects_are_still_defects`, + pointed the other way -- there an entry must keep reproducing, here a + key must keep matching. + + A deliberate rewording is meant to fail this and have its entry removed + or updated, which is a one-line edit made with the reason fresh. + + """ + quoted = [' '.join(doc.split()).lower() for _, _, doc in docstrings()] + for sentence in RFC8200_QUOTED_SENTENCES: + with self.subTest(sentence=sentence[:60]): + self.assertTrue(any(sentence in doc for doc in quoted), + 'no docstring quotes this any more, so its entry ' + 'in RFC8200_QUOTED_SENTENCES checks nothing -- ' + 'update or remove it') + + if __name__ == '__main__': unittest.main()