diff --git a/CHANGELOG.md b/CHANGELOG.md index 838c2e1643..b18d8db435 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,7 +60,7 @@ This is the resolution of #548, which reported `TransType.L2TP` (115) as registe - **Added** -- `tests/protocols/test_option_generator_tcp_base_unit.py`, asserting that every `TCP_BASE` key is a parameter `TCP.make` declares -- derived from `inspect.signature`, so it catches a fourth misspelling nobody has made yet -- and that the segment the mapping builds carries the stated header in both the data model and the packed octets. Two of the three wrong names were invisible to any assertion about a *value*, the value asked for being equal to the default that was used instead, which is what the signature check is for (#602). - **Fixed** -- the ILNP Nonce option builders in `HOPOPT` and `IPv6_Opts` sized the option with `math.ceil(nonce.bit_length() // 8)`, which is floor division dressed up as a ceiling: `//` floors, and `math.ceil` of an `int` is a no-op, so the ceiling was never actually taken. The nonce is packed by a `NumberField` whose width *is* that declared `len`, so an under-declared length did not merely mis-state the option -- it silently truncated the nonce on the wire, with nothing raised. Every nonce whose bit length was not an exact multiple of eight was affected, and **small values were the worst case rather than boundary values**: any nonce below 256 was declared as *zero* octets and dropped from the packet altogether, so `nonce=9` packed to `b'\x8b\x00'` and parsed back as `0`, while `nonce=256` and `nonce=65536` each truncated to `0` as well. Fixed to `max(1, math.ceil(nonce.bit_length() / 8))`, the form already used at five other sizing sites across `hip.py` and `mh.py`. The one-octet floor is the `mh.py` convention and is load-bearing here because `nonce` defaults to `0`, whose bit length is `0`: without it the default argument builds an ILNP Nonce option carrying no Nonce Value field at all, collapsing "the nonce is 0" into "there is no nonce" when [RFC 6744](https://datatracker.ietf.org/doc/html/rfc6744) gives the option that field. The read path was never affected, since it takes the width from the `len` octet on the wire rather than recomputing it (#601). - **Added** -- ILNP nonce sizing coverage in `tests/protocols/internet/test_ipv6_extension_unit.py`, one test per protocol, asserting the declared length, the exact packed octets and the construct-pack-parse cycle over ten nonces. The reason the existing suite missed this is that the only ILNP nonce it ever exercised was `0xFFFFFF` -- bit length 24, an exact multiple of eight, precisely where floor division and the ceiling agree -- the same blind spot that hid the identical typo in `numbers.py` behind bit lengths 8, 16, 24, 32 and 64. Every new case bar two deliberate controls therefore has a bit length that is *not* a multiple of eight, several of them below 256. The table also guards itself: the test asserts that at least six of its own values stay non-byte-aligned and that one stays below 256, so rounding them off to convenient constants later cannot quietly disarm the regression (#601). -- **Fixed** -- the string-keyed `get()` in `pcapkit.const.ftp.command` and `pcapkit.const.http.method` tested membership with the raw key but registered `key.upper()`, so the *first* lowercase or mixed-case token raised `TypeError: 'RETR' already in use` rather than resolving. Reachable from wire data for FTP: `pcapkit.protocols.application.ftp` compiles its request pattern with `re.I` and passes the match verbatim, and [RFC 959 Section 5.3](https://datatracker.ietf.org/doc/html/rfc959#section-5.3) makes FTP commands case-insensitive -- "Upper and lower case alphabetic characters are to be treated identically", listing `RETR Retr retr ReTr rETr` as the same command -- so `retr file.txt` was a valid request this library could not parse. Both `get()` and `_missing_` now look the key up under the same canonical upper-case name they register it under, so every casing resolves to the one member that already exists instead of colliding with it. Resolving rather than registering a second member matters beyond not crashing -- a duplicate `GET` would carry neither the `safe` nor the `idempotent` attribute of the real one (#582, #583). +- **Fixed** -- the string-keyed `get()` in `pcapkit.const.ftp.command` and `pcapkit.const.http.method` tested membership with the raw key but registered `key.upper()`, so the *first* lowercase or mixed-case token raised `TypeError: 'RETR' already in use` rather than resolving. Reachable from wire data for FTP: `pcapkit.protocols.application.ftp` compiles its request pattern with `re.I` and passes the match verbatim, and [RFC 959 Section 5](https://datatracker.ietf.org/doc/html/rfc959#section-5) makes FTP commands case-insensitive -- "Upper and lower case alphabetic characters are to be treated identically", listing `RETR Retr retr ReTr rETr` as the same command -- so `retr file.txt` was a valid request this library could not parse. Both `get()` and `_missing_` now look the key up under the same canonical upper-case name they register it under, so every casing resolves to the one member that already exists instead of colliding with it. Resolving rather than registering a second member matters beyond not crashing -- a duplicate `GET` would carry neither the `safe` nor the `idempotent` attribute of the real one (#582, #583). - **Fixed** -- `httpv1`'s `_RE_METHOD` was unanchored and `re.match` anchors only at the start, so it prefix-matched, and the request-line reader then passed the whole `para1` to `Method.get` rather than the captured `method` group. Together those meant `b'Get'` matched on the single character `G`, satisfied the guard that decides a start-line is a request, and handed the entire mixed-case token to a lookup that raised on it. Fixing either half alone still gives a wrong answer -- normalising the lookup would parse `b'Get'` as `GET` off a one-character match, and passing the group would parse it as a method named `G`. The pattern is now anchored at both ends and the captured group is what is looked up, so a token that is not a method is a malformed request line rather than a mis-parsed one. Method tokens are case-sensitive per [RFC 9110 Section 9.1](https://datatracker.ietf.org/doc/html/rfc9110#section-9.1), so no `re.I` was added: `GET` parses, `Get` and `get` are rejected (#583). - **Fixed** -- `_RE_STATUS` in the same reader carried the same unanchored prefix defect, found by auditing `_RE_METHOD`'s siblings, and it escaped as the wrong exception type. That pattern is only a guard -- the value is taken from `int(para2)` on the raw token -- so a prefix match let a malformed status past the guard and then out of `int()` uncaught, where `_read_http_header` documents `ProtocolError`. Measured: a status of `200x` raised `ValueError: invalid literal for int() with base 10: b'200x'`, and one of `2000` raised `ValueError: 2000 is not a valid StatusCode`; both are now `ProtocolError`. [RFC 9112 Section 4](https://datatracker.ietf.org/doc/html/rfc9112#section-4) gives `status-code = 3DIGIT`, exactly three, so the anchor is what the grammar already said -- the production lives in HTTP/1.1 because `status-code` is part of its `status-line`, while [RFC 9110 Section 15](https://datatracker.ietf.org/doc/html/rfc9110#section-15) covers the code semantics and the IANA registry rather than the syntax. `_RE_VERSION` was audited at the same time and is safe as it stands, because both of its call sites read the captured group rather than the raw token (#583). - **Fixed** -- `get()`'s documented `default` was ignored on the integer path throughout the generated `pcapkit.const` tree, because `get` delegated the lookup to the enum call and `_missing_` has no access to the caller's `default` -- so `Hardware.get(99999, 0)` raised `ValueError: 99999 is not a valid Hardware` instead of returning the fallback it was handed. The integer path now consults `default` before letting the lookup error escape. `-1`, the placeholder the generated signature already carried, is what separates "no default was supplied" from "a default was supplied and should be used", so a caller that asked for no fallback still gets the error rather than a silent substitution. The sweep #584 asked for puts the scope at 110 of the 118 integer registries, not the three the issue named; the two carrying a bespoke integer fallback of their own, `pcapng` `OptionType` and `reg` `AppType`, are deliberately left alone, since neither drops a default by raising. Not reachable from wire data -- every value a wire field can carry already resolves -- so this is a contract fix rather than a parse fix. Applied to the nine vendor templates as well as the 113 generated modules, and a new test renders the shared template and compares it against the module generated from it, so a regeneration cannot quietly undo it (#584). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 7486ebde11..9464f21973 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -756,7 +756,7 @@ pull requests between #326 and #509. raised ``TypeError: 'RETR' already in use`` rather than resolving. Reachable from wire data for FTP: ``pcapkit.protocols.application.ftp`` compiles its request pattern with ``re.I`` and passes the match verbatim, and - :rfc:`959#section-5.3` makes FTP commands case-insensitive -- "Upper and lower + :rfc:`959#section-5` makes FTP commands case-insensitive -- "Upper and lower case alphabetic characters are to be treated identically", listing ``RETR Retr retr ReTr rETr`` as the same command -- so ``retr file.txt`` was a valid request this library could not parse. Both ``get()`` and ``_missing_`` diff --git a/docs/source/contributing/conventions/registry-protocol.rst b/docs/source/contributing/conventions/registry-protocol.rst index fad5742f66..3efa27695c 100644 --- a/docs/source/contributing/conventions/registry-protocol.rst +++ b/docs/source/contributing/conventions/registry-protocol.rst @@ -221,7 +221,7 @@ about spelling**? The owner's answer, verbatim: So the test a new registry has to pass has **two limbs**, and satisfying either one justifies case-insensitivity: -1. **A comparison rule in the governing document.** :rfc:`959#section-4.1` for FTP +1. **A comparison rule in the governing document.** :rfc:`959#section-5` for FTP command codes, :rfc:`5797#section-2` for FTP FEAT codes, :rfc:`6335#section-5.1` for IANA service names. 2. **A documented spelling disagreement between the specification and the registry.** @@ -278,7 +278,7 @@ That leaves the classes with something to decide: - What it says - Verdict * - :class:`~pcapkit.const.ftp.command.Command` - - :rfc:`959#section-4.1` + - :rfc:`959#section-5` - *"Upper and lower case alphabetic characters are to be treated identically."* Limb 1. - **case-insensitive** -- ``get``/``_missing_`` fold, correctly @@ -334,7 +334,7 @@ That leaves the classes with something to decide: are** service names. - **unimplemented** -- no service-name lookup exists to fold; see below * - ``CommandType``, ``ConformanceRequirement`` - - :rfc:`959#section-4.1`, :rfc:`5797#section-2` + - :rfc:`959#section-4`, :rfc:`5797#section-2` - Limb 2 holds on measurement: the RFC and registry pages present the kind and conformance letters upper case (``A``/``P``/``S``, ``M``/``O``/``H``) while the CSV columns the crawler reads are lower case in every row (``s`` 26, @@ -440,7 +440,7 @@ rather than changing it. fall-through can be measured on a class that defines no ``get``. ``Command.get`` upper-cases its key before matching, which makes it look as though the base were case-insensitive -- - deliberately, since :rfc:`959#section-4.1` treats FTP command codes identically + deliberately, since :rfc:`959#section-5` treats FTP command codes identically regardless of case. ``Method.get`` used to fold case the same way, but `#896 `__ made it case-sensitive instead: :rfc:`9110#section-9.1` says the HTTP method token is diff --git a/pcapkit/const/ftp/command.py b/pcapkit/const/ftp/command.py index 7861e44669..a8d538d200 100644 --- a/pcapkit/const/ftp/command.py +++ b/pcapkit/const/ftp/command.py @@ -184,7 +184,7 @@ def _missing_(cls, value: 'str') -> 'FEATCode': class CommandType(EnumLookup, IntFlag): - """Type of "kind" of command, based on :rfc:`959#section-4.1`. + """Type of "kind" of command, based on :rfc:`959#section-4`. Re-parented onto :class:`~pcapkit.corekit.enum.EnumLookup` per GitHub issue #930, finishing #877's phase 2. Pure re-parenting as far as @@ -258,7 +258,7 @@ class Command(EnumRegistry, StrEnum): feat: 'Optional[FEATCode]' #: Brief description of command / extension. desc: 'Optional[str]' - #: Type of "kind" of command, based on :rfc:`959#section-4.1`. + #: Type of "kind" of command, based on :rfc:`959#section-4`. type: 'CommandType' #: Expectation for support in modern FTP implementations. conf: 'ConformanceRequirement' diff --git a/pcapkit/const/http/method.py b/pcapkit/const/http/method.py index 0e0ae483ab..e73bb24617 100644 --- a/pcapkit/const/http/method.py +++ b/pcapkit/const/http/method.py @@ -265,7 +265,7 @@ def get(cls, key: 'str', default: 'Optional[str]' = None) -> 'Method': per :rfc:`9110#section-9.1` -- the method token is case-sensitive, unlike :meth:`~pcapkit.const.ftp.command. Command.get`'s equivalent override, which stays - case-insensitive because :rfc:`959#section-4.1` says FTP + case-insensitive because :rfc:`959#section-5` says FTP command codes are not. Checked against both member names and values, via the base's own precedence -- name before value -- so a value-only match such as diff --git a/pcapkit/vendor/ftp/command.py b/pcapkit/vendor/ftp/command.py index 50d569fbf4..e97c9d7abc 100644 --- a/pcapkit/vendor/ftp/command.py +++ b/pcapkit/vendor/ftp/command.py @@ -196,7 +196,7 @@ def _missing_(cls, value: 'str') -> 'FEATCode': class CommandType(EnumLookup, IntFlag): - """Type of "kind" of command, based on :rfc:`959#section-4.1`. + """Type of "kind" of command, based on :rfc:`959#section-4`. Re-parented onto :class:`~pcapkit.corekit.enum.EnumLookup` per GitHub issue #930, finishing #877's phase 2. Pure re-parenting as far as @@ -270,7 +270,7 @@ class {NAME}(EnumRegistry, StrEnum): feat: 'Optional[FEATCode]' #: Brief description of command / extension. desc: 'Optional[str]' - #: Type of "kind" of command, based on :rfc:`959#section-4.1`. + #: Type of "kind" of command, based on :rfc:`959#section-4`. type: 'CommandType' #: Expectation for support in modern FTP implementations. conf: 'ConformanceRequirement' diff --git a/pcapkit/vendor/http/method.py b/pcapkit/vendor/http/method.py index eff8fe0a95..f477aece50 100644 --- a/pcapkit/vendor/http/method.py +++ b/pcapkit/vendor/http/method.py @@ -170,7 +170,7 @@ def get(cls, key: 'str', default: 'Optional[str]' = None) -> '{NAME}': per :rfc:`9110#section-9.1` -- the method token is case-sensitive, unlike :meth:`~pcapkit.const.ftp.command. Command.get`'s equivalent override, which stays - case-insensitive because :rfc:`959#section-4.1` says FTP + case-insensitive because :rfc:`959#section-5` says FTP command codes are not. Checked against both member names and values, via the base's own precedence -- name before value -- so a value-only match such as diff --git a/tests/const/test_const_enum_no_mint.py b/tests/const/test_const_enum_no_mint.py index 7f79417b1c..f62d2ccb5a 100644 --- a/tests/const/test_const_enum_no_mint.py +++ b/tests/const/test_const_enum_no_mint.py @@ -2108,7 +2108,7 @@ def test_registered_lookups_are_still_unaffected(self) -> None: ``Method.get`` is probed with its exact registered casing (``'GET'``), not the lower-cased ``'get'`` this test used before GitHub issue #896: ``Command``'s FTP command codes stay - case-insensitive per :rfc:`959#section-4.1`, but the HTTP method + case-insensitive per :rfc:`959#section-5`, but the HTTP method token :rfc:`9110#section-9.1` covers is case-sensitive, so ``Method.get('get')`` no longer resolves to :attr:`Method.GET` -- see :class:`BespokeGetUnchangedTests`'s diff --git a/tests/project/test_rfc_anchor_fragments.py b/tests/project/test_rfc_anchor_fragments.py index 876747766f..0f37c81745 100644 --- a/tests/project/test_rfc_anchor_fragments.py +++ b/tests/project/test_rfc_anchor_fragments.py @@ -71,13 +71,37 @@ today, so this changes nothing here; rejecting is the more conservative default should either turn up later. -One thing this test does not check: whether the anchor actually exists on the -RFC's own page, only whether it is *shaped* like one could -- filed -separately as GitHub issue #944 for the live instance this sweep does not -catch, ``:rfc:`959#section-4.1``` (RFC 959 has only ``section-1`` through -``section-8``; 4 of its 6 sites are in the two ``ftp/command.py`` files this -change touches), left for that issue's own editorial call rather than guessed -at here. +One thing the shape check above does not catch: whether the anchor actually +exists on the RFC's own page, only whether it is *shaped* like one could -- +that is exactly the class GitHub issue #944 found, ``:rfc:`959#section-4.1``` +(RFC 959's own rendered HTML carries only ``section-1`` through ``section-8``; +verified by reading both ``https://www.rfc-editor.org/rfc/rfc959.html`` and +the datatracker mirror). The maintainer ruled on #944 for the enclosing +top-level section, so those citations became ``:rfc:`959#section-4``` -- +which :data:`ACCEPTED_FRAGMENT` above already accepts as well-formed. + +Eleven sites carried it, not the six #944 reported: that issue grepped +:mod:`pcapkit` only, and a cross-review of the fix found four more rendered +roles in :file:`docs/source/contributing/conventions/registry-protocol.rst` +and one in :file:`tests/const/test_const_enum_no_mint.py`. A further two +cited ``959#section-5.3``, equally dead, in +:file:`tests/protocols/application/test_ftp_unit.py` and +:file:`docs/source/changelog/1.5.0.rst`; #947 ruled ``#section-5`` for those. +**Every** sub-numbered ``959`` fragment is dead, since that RFC renders +anchors for its eight top-level sections and nothing finer -- which is the +general statement, and the reason the denylist below carries both fragments +rather than only the reported one. + +:data:`KNOWN_DEAD_ANCHORS` and :meth:`RFCAnchorFragmentTests. +test_no_known_dead_anchor_citations` below add a second, narrower check for +that same defect class: a **denylist** of RFC-number -> anchor pairs already +confirmed, by reading that RFC's own rendered HTML, not to exist -- starting +with the two ``959`` fragments already confirmed dead, so neither can come +back unnoticed. This is deliberately a denylist, not an existence oracle: it +proves nothing about any fragment not already listed here, and adding a new +RFC/anchor pair always means someone read that RFC's actual HTML first -- +never a guess extrapolated from this test alone. It stays offline (no network +call) so it keeps running under this repository's CI, which has none. """ @@ -105,6 +129,60 @@ ACCEPTED_FRAGMENT = re.compile( r'\A(?:section-\d+(?:\.\d+)*|appendix-[A-Z](?:\.\d+)*|page-\d+)\Z') +#: A **denylist** of RFC number -> the set of anchor fragments that RFC's own +#: rendered HTML is *known* not to have, confirmed by reading +#: ``https://www.rfc-editor.org/rfc/rfc.html`` (or the datatracker mirror) +#: rather than by any crawl or heuristic. This cannot catch an arbitrary dead +#: anchor -- only ones already found and added here -- so its absence from +#: this table is not evidence a fragment is live; see the module docstring's +#: closing paragraphs for why that limitation is deliberate. Both ``959`` entries +#: come from GitHub issue #944: RFC 959's HTML carries anchors only for its eight +#: top-level sections, ``section-1`` through ``section-8``, so *every* sub-numbered +#: ``959`` fragment is dead. ``section-4.1`` is the one #944 reported; +#: ``section-5.3`` turned up in the same sweep, cited for the case-insensitivity +#: rule it does correctly name in prose. +KNOWN_DEAD_ANCHORS: 'dict[int, frozenset[str]]' = { + 959: frozenset({'section-4.1', 'section-5.3'}), +} + +#: The exact ways this repository writes a ``:rfc:`` citation in order to *name* a +#: defect rather than commit one, as ``(prefix, suffix)`` around the role text. A hit +#: matching any of these is documentation, not a live link. +#: +#: The first entry is the inline-literal idiom -- ``` ``:rfc:`959#section-4.1``` ``` -- +#: where the literal's content is itself a role, so the opening delimiter is two +#: backticks and the close is three: two for the literal plus the role's own trailing +#: one. That asymmetry is why the list holds prefix/suffix pairs rather than a single +#: delimiter. +#: +#: **This list replaced a general masker, and the reason is the point.** Deciding +#: "is this text inside any inline literal" over the whole tree was tried five times +#: and was wrong five times -- a lookbehind heuristic, then a ``re.DOTALL`` span that +#: crossed paragraph breaks and hid a live ``:rfc:`4303#section-2.1``` role, then a +#: greedy close that swallowed the next literal's opener, then a role's own backtick +#: fusing with an adjacent literal. Each fix addressed the mechanism the previous +#: postmortem had found and missed the next. The general problem is RST's +#: inline-markup grammar, which a regex was never going to model. +#: +#: The bounded question is a different size. Rather than classifying every literal in +#: the tree -- **23,869** spans across **926** files, every one of which had to be got +#: right -- it asks only where a *denylisted fragment* appears, which is **2** +#: occurrences today, and classifies each against this list. Adding a form means one +#: reviewed entry, the same scrutiny :data:`KNOWN_DEAD_ANCHORS` entries already get. +QUOTED_FORMS: 'tuple[tuple[str, str], ...]' = ( + ('``', '``'), +) + +#: Where :func:`_dead_anchor_citations` looks, as ``(directory, glob patterns)``. +#: Wider than :func:`_malformed_fragments`'s ``pcapkit``-only sweep on purpose: +#: a dead anchor in a ``.rst`` page renders for readers exactly as one in a +#: docstring does, and four of the five sites this caught were in ``.rst``. +DEAD_ANCHOR_SCAN: 'tuple[tuple[str, tuple[str, ...]], ...]' = ( + ('pcapkit', ('*.py',)), + ('docs/source', ('*.rst', '*.py')), + ('tests', ('*.py',)), +) + class Finding(NamedTuple): """One malformed ``:rfc:`` fragment, keyed so it survives lines moving.""" @@ -115,6 +193,17 @@ class Finding(NamedTuple): fragment: 'str' +class DeadAnchorFinding(NamedTuple): + """One citation of a fragment listed in :data:`KNOWN_DEAD_ANCHORS`.""" + + #: Path relative to the repository root. + path: 'str' + #: The RFC number cited. + rfc: 'int' + #: The fragment text itself (after ``#``, before the closing backtick). + fragment: 'str' + + def _malformed_fragments() -> 'list[Finding]': """Every ``:rfc:`` fragment under :mod:`pcapkit` shaped unlike a real anchor.""" findings = [] @@ -127,8 +216,107 @@ def _malformed_fragments() -> 'list[Finding]': return findings +def _is_documentation(text: 'str', start: 'int', length: 'int') -> 'bool': + """Whether the citation at ``start`` is one of :data:`QUOTED_FORMS`. + + Local and exact: it looks only at the characters immediately either side of this + one citation, so it cannot be thrown off by markup elsewhere in the file. That is + the whole advantage over the masker it replaced, which decided the same question by + pairing every literal delimiter in the file and got it wrong whenever the pairing + slipped. + + Args: + text: The file's contents. + start: Index where the citation begins. + length: The citation's length. + + Returns: + Whether it is being named rather than committed. + + """ + for prefix, suffix in QUOTED_FORMS: + opener = start - len(prefix) + if opener < 0 or text[opener:start] != prefix: + continue + if text[start + length:start + length + len(suffix)] != suffix: + continue + # The opener has to *begin* a token, and only whitespace or the start of the + # file may precede it. + # + # An earlier version also allowed ``([{"'`` there, on the reasoning that an + # opening bracket or quote may sit before a literal. That was **the sixth + # mechanism**, and it was mine: those characters are equally valid as the last + # character of a literal's *content*, in which case the ``` `` ``` is a + # **closer** and the role after it is live. So ``` ``'base'``:rfc:`959#…`` ``` + # read as documentation -- and ``` ``'base'`` ``` is a real literal in + # :file:`pcapkit/const/ftp/command.py`. Measured across the scanned roots, + # **764** literals in **188** files end in one of those five characters, so the + # exemption was one adjacency away from hiding a live dead link. + # + # Whitespace and start-of-file are safe by RST's own grammar: an inline + # literal's *end*-string cannot be preceded by whitespace, so ``` `` ``` + # following whitespace is always an opener. + # + # The comment this replaced claimed the condition was "measured, not reasoned + # about". It had been measured only against ``` ``foo`` ``` -- a literal ending + # in a letter -- and never against the characters in its own allowed set, which + # is exactly where it failed. A measurement that skips the interesting inputs + # is reasoning wearing a measurement's clothes. + if opener == 0 or text[opener - 1].isspace(): + return True + return False + + +def _dead_anchor_citations() -> 'list[DeadAnchorFinding]': + """Every live citation of a fragment listed in :data:`KNOWN_DEAD_ANCHORS`. + + Offline by construction -- it consults the fixed table above and never the + network -- so it runs the same way in CI, which has none. + + Searches for each denylisted fragment as a **literal string** rather than matching + every ``:rfc:`` role and comparing. Two consequences worth stating, because the + previous design had neither: + + * The work is proportional to the number of *denylisted* fragments present, not to + the amount of markup in the tree. There is no classifier to get wrong on a file + that has nothing to do with these anchors. + * A citation this table has never heard of is not examined at all. That is not a + regression -- the denylist never made a claim about one -- but it means this + function answers a narrower question than its predecessor pretended to. + + Scans :data:`DEAD_ANCHOR_SCAN` rather than :mod:`pcapkit` alone: scanning one + directory is what let five dead-anchor sites sit unnoticed outside it, four of them + in ``.rst`` where they rendered for readers exactly as a docstring's would. + + """ + findings = [] + for root, patterns in DEAD_ANCHOR_SCAN: + base = ROOT / root + for pattern in patterns: + for path in sorted(base.rglob(pattern)): + text = path.read_text(encoding='utf-8') + for rfc, fragments in KNOWN_DEAD_ANCHORS.items(): + for fragment in sorted(fragments): + needle = f':rfc:`{rfc}#{fragment}`' + start = text.find(needle) + while start != -1: + if not _is_documentation(text, start, len(needle)): + findings.append(DeadAnchorFinding( + str(path.relative_to(ROOT)), rfc, fragment)) + start = text.find(needle, start + 1) + return findings + + class RFCAnchorFragmentTests(unittest.TestCase): - """No ``:rfc:`` role under :mod:`pcapkit` may cite a malformed fragment.""" + """No ``:rfc:`` role may cite a malformed fragment, or a known-dead anchor. + + The two checks have **different scopes**, deliberately. + :meth:`test_no_malformed_fragments` sweeps :mod:`pcapkit` only; + :meth:`test_no_known_dead_anchor_citations` sweeps :data:`DEAD_ANCHOR_SCAN`, which + also covers :file:`docs/source` and :file:`tests`, because scanning one directory + is what let five dead-anchor sites sit unnoticed outside :mod:`pcapkit`. + + """ def test_no_malformed_fragments(self) -> 'None': """No ``:rfc:`` fragment under :mod:`pcapkit` is shaped like a typo. @@ -147,6 +335,115 @@ def test_no_malformed_fragments(self) -> 'None': f'malformed :rfc: fragment(s) found under pcapkit/: {found}', ) + def test_the_documentation_exemption_holds_on_every_known_mechanism(self) -> 'None': + """:func:`_is_documentation` on each input that defeated the old masker. + + This is the regression suite for five rounds of review, and it exists because + each of those rounds fixed the mechanism the previous postmortem had named and + then missed the next one. Pinning the *table* rather than the latest fix is the + difference: a sixth mechanism has to be added here to be considered handled, and + a redesign has to keep every row passing. + + The rows are the real inputs, not paraphrases of them. Four are false negatives + the masker produced -- a live role it hid -- which is the dangerous direction, + since a hidden role is a dead link nobody sees. One is the documentation idiom + that must stay exempt, or the defect becomes impossible to write about in the + very file that guards against it. + + """ + # Built from parts so this file does not itself contain a live-looking + # citation of a denylisted fragment -- writing the test data literally trips + # the very guard under test, which is the same "documenting the defect is the + # hazard" problem QUOTED_FORMS exists for, one layer up. + needle = ':rfc:`%d#%s`' % (959, 'section-4.1') + cases = ( + ('a plain live role', 'cites %s here', True), + ('the documentation idiom', 'names ``%s`` here', False), + ('a live role after a closing inline literal', + 'See ``foo``%s` dead.', True), + ('a live role after a title reference', '`RFC 959`%s dead.', True), + ('a live role several paragraphs after a literal', + 'Prose ``x``.\n\np2\n\np3\n\ncites %s live\n', True), + ('a live role after two adjacent literals', '``a````b`` %s ``c``', True), + ('a live role flush against a following literal', + 'cite %s``next`` after', True), + ('a live role flush against a spaced literal', + 'cite %s` ``next`` after', True), + # Mechanism 6: the five characters an earlier boundary check allowed before + # the opener are equally valid as a literal's last *content* character, in + # which case the delimiter is a closer and the role is live. One row each, + # because they were allowed as a set and had to be refuted as a set. + ("a live role after a literal whose content ends in a quote", + "See ``x'``%s`` dead.", True), + ('a live role after a literal whose content ends in a paren', + 'See ``foo(``%s`` dead.', True), + ('a live role after a literal whose content ends in a bracket', + 'See ``foo[``%s`` dead.', True), + ('a live role after a literal whose content ends in a brace', + 'See ``foo{``%s`` dead.', True), + ('a live role after a literal whose content ends in a double quote', + 'See ``f"``%s`` dead.', True), + # The real one: ``'base'`` is an inline literal in + # pcapkit/const/ftp/command.py, and 764 literals across 188 files end in + # one of those five characters, so this shape is one adjacency away. + ("a live role after this repository's own quoted literal", + "See ``'base'``%s`` dead.", True), + # Pins the suffix half of the (prefix, suffix) pair. Deleting the suffix + # condition passed every row above, so without this the reason + # QUOTED_FORMS holds pairs at all is unasserted. + ('a citation opened like the idiom but not closed like it', + 'names ``%s and more', True), + # Pins **any** whitespace rather than a literal space. Narrowing + # ``isspace()`` to ``== ' '`` passed all fifteen earlier rows, because the + # only row reaching that branch used a plain space -- so the boundary + # condition the comment above relies on was asserted for one whitespace + # character out of the set it names. A reader writing the idiom after a + # line wrap, or first on an indented line, hits exactly these. + ('the idiom preceded by a tab', 'names\t``%s`` here', False), + ('the idiom preceded by a newline', 'names\n``%s`` here', False), + ) + cases = tuple((label, template % needle, live) + for label, template, live in cases) + + for label, text, expected_live in cases: + with self.subTest(case=label): + live = [index for index in range(len(text)) + if text.startswith(needle, index) + and not _is_documentation(text, index, len(needle))] + self.assertEqual( + bool(live), expected_live, + f'{label}: expected the citation to be treated as ' + f'{"live" if expected_live else "documentation"} and it was not -- ' + 'a live role read as documentation is a dead link nobody sees') + + def test_no_known_dead_anchor_citations(self) -> 'None': + """No ``:rfc:`` role anywhere in :data:`DEAD_ANCHOR_SCAN` cites a known-dead anchor. + + A well-formed fragment (one :meth:`test_no_malformed_fragments` above + already accepts) can still name an anchor its RFC does not render -- + that is #944's defect, ``:rfc:`959#section-4.1```, which the shape + check cannot see because ``section-4.1`` is a perfectly well-shaped + fragment. This test catches that *narrower* class by checking every + role in the scanned roots against :data:`KNOWN_DEAD_ANCHORS`, a + denylist of RFC number -> anchors already confirmed dead by reading + that RFC's own rendered HTML. + + This is a denylist, not an oracle: passing this test is not proof + that every ``:rfc:`` citation in the tree resolves, only that none of + them cite one of the specific, already-known-dead anchors listed in + :data:`KNOWN_DEAD_ANCHORS`. An anchor dead in some RFC this table has + never heard of would sail through unnoticed -- deliberately, since + the alternative is a live network fetch this offline test (and this + repository's CI, which has none) cannot make. + + """ + found = _dead_anchor_citations() + + self.assertEqual( + found, [], + f'known-dead RFC anchor cited as a live role: {found}', + ) + #: Matches the ``FEATCode`` class header regardless of its base classes, #: so a future re-parenting (like #930/#932/#937 did to five siblings) #: does not fail this pin for a reason unrelated to the RFC citation -- diff --git a/tests/protocols/application/test_ftp_unit.py b/tests/protocols/application/test_ftp_unit.py index 0e8d64afdf..8e5341d150 100644 --- a/tests/protocols/application/test_ftp_unit.py +++ b/tests/protocols/application/test_ftp_unit.py @@ -87,19 +87,22 @@ def test_command_get_is_case_insensitive(self) -> None: looked up, so the first lowercase command raised ``TypeError`` instead of resolving -- #582. - :rfc:`959#section-5.3` makes FTP commands case-insensitive -- "Upper and + :rfc:`959#section-5` makes FTP commands case-insensitive -- "Upper and lower case alphabetic characters are to be treated identically. Thus, any of the following may represent the retrieve command: ``RETR Retr retr ReTr rETr``" -- so every casing of a registered command has to resolve to the *same* member rather than to a second one registered alongside it. - The citation is section 5.3 (COMMANDS, which gives the command syntax), + The rule is in section 5.3 (COMMANDS, which gives the command syntax), not section 4.1 (FTP COMMANDS, which only lists the per-command semantics); #582 cited 4.1 and a cross-review caught it. Verified against the RFC text: the sentence sits between the 5.3 and 5.4 headings. The - ``:rfc:`959#section-4.1``` citations in - :mod:`pcapkit.const.ftp.command` are a different claim -- the command - *kind* (access control, transfer parameter, service) -- and are correct. + *link* above says ``#section-5`` rather than ``#section-5.3`` because RFC + 959 renders anchors only for its eight top-level sections -- GitHub issue + #944 -- so ``#section-5.3`` was a live link to nothing. The + ``:rfc:`959#section-4``` citations in :mod:`pcapkit.const.ftp.command` are + a different claim -- the command *kind* (access control, transfer + parameter, service) -- and are correct. """ from pcapkit.const.ftp.command import Command