Skip to content

fix(tcp): enforce the SACK length rule its docstring already promised (#519) - #535

Merged
JarryShaw merged 2 commits into
mainfrom
fix-519
Sep 20, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix-519

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Closes #519.

#501 fixed one wrong exception name and forty stale Args: labels by hand. #519 showed the class was not exhausted. Re-deriving its census at the current base turned up one finding that is not a docstring defect at all, plus a third defect class the issue never mentions.

The one behavioural change, and why it is not a docstring deletion

TCP._read_mode_sack documented ProtocolError: If length is **NOT** multiply of 8 plus 2 and never checked it. The tempting reading — and the first one taken while working this — is that the clause was stale, like the other five phantoms, and should be deleted. It is the opposite. Three things settle it:

  • :rfc:2018 gives the SACK option a 2-octet header followed by 8-octet left/right edge pairs, so a well-formed length really is 8n + 2.
  • Both neighbouring readers validate their own lengths rather than delegating: _read_mode_sackpmt on length != 2, _read_mode_echo on length != 6, with the same message. _read_mode_sack was the one option reader documenting a length rule it never enforced.
  • Nothing upstream enforced it either. The schema field is ListField(length=lambda pkt: pkt['length'] - 2, item_type=SchemaField(length=8, schema=SACKBlock)), which consumes as many whole 8-octet items as it finds and silently ignores the tail.

So the docstring was right and the code was wrong. Inferring the behaviour from a neighbouring docstring — inside an issue about docstrings not matching behaviour — is exactly the trap here, and fuzzing the real parse path is what caught it.

Re-derived census

Taken at this base, not from the issue, which counted against an older one:

class at c8fd97bcd remaining
phantom Raises: 6 0
Args: naming a parameter that does not exist 13 9, all allowlisted
swallowed section headers 3 1, allowlisted

The Args: figure is 13 here and was 14 when #519 was written: #511 retired the IPX socket scrape and renamed that crawler's soup parameter to data, incidentally fixing one. The issue's own figure of 11 undercounted by three.

The nine that remain sit in files owned by other in-flight changes (#506, #524, #527, #531, and the registry docstring change) and are recorded in KNOWN_DEFECTS with a reason each, under a test asserting every one still reproduces — so the list cannot rot into a description of bugs that are gone. That test has already earned its keep: it failed on exactly the IPX entry during the rebase, which is how the stale entry was found and removed.

What can and cannot fail a test — both framings, honestly

The docstring corrections cannot fail a test individually. A label is not executed, so there is no fails-before to show for them and none is manufactured here. What pins them is tests/test_docstring_contract.py, which derives the answer from the code rather than snapshotting today's wrongness — so a docstring written next year is checked next year. Verified by reintroducing each class into a scratch tree:

baseline, all fixes in place          exit 0
phantom Raises: reintroduced         exit 1   "1 phantom Raises: clause"
bad Args: label reintroduced         exit 1   "1 docstring(s) name a parameter that does not exist"
Returns: re-indented into Args:      exit 1   "1 section header(s) indented into a parameter block"

Two of those are not merely cosmetic, and both were measured rather than argued:

  • The swallowed Returns: loses documentation from the built page. Run through napoleon's GoogleDocstring directly, the over-indented form yields :param Returns: and no :returns: at all; the fixed form yields :returns:. napoleon_include_private_with_doc = True in docs/source/conf.py, so these private methods do publish.
  • The Args: mismatches are hard TypeErrors. None of the four functions takes **kwargs. Frame.register(code=..., module=IPv4) really does raise TypeError: Frame.register() got an unexpected keyword argument 'module', while protocol= is accepted.

The SACK check can and does fail before the fix. Lengths 3, 11, 14, 17, 19 and 25 all parsed clean on c8fd97bcd with the SACK option present in tcp.info['options']; all six now raise ProtocolError: TCP: [OptNo 5] invalid format. Independently reproduced in a throwaway worktree at origin/main with only the new test file copied in: exit 1 with SUBFAILED for all six lengths, against exit 0 on this branch.

The two Raises: assertions are complementary, not redundant, and _read_http_none proves it: any(header.flags) in its body makes the conservative reachability check judge it possibly-raising, so only the commented-out-raise check catches it. Measured — reachability exit 0, commented-raise exit 1 with httpv2.py:417 _read_http_none() documents 'ProtocolError' but its only raise is commented out. Delete either test and one of the six phantoms stops being covered.

Deliberately not done

  • **kwargs completeness in hip.py. All 12 functions carrying both **kwargs and an Args: section without documenting **kwargs are in hip.py; 438 of 450 document it elsewhere. That set of 12 includes _make_param_reg_failed and _make_param_route_via, the very siblings this change copies its wording from — so fixing 4 of the 12 is what would make hip.py inconsistent. Left whole, reported rather than half-fixed.
  • RFC 2018's block-count bound. The check implements exactly the documented rule, (length - 2) % 8 == 0. length=2 is degenerate and still parses; five blocks or more cannot be expressed at all, since the 4-bit data offset caps the options area at 40 octets. Recorded in the test as a decision rather than quietly tightened past the docstring.
  • Reported, not fixed (other streams own the files): Args: mismatches at pcapkit/vendor/ipx/packet.py, four pcapkit/vendor/mh/*, pcapkit/vendor/pcapng/option_type.py, pcapkit/vendor/vlan/priority_level.py, pcapkit/foundation/registry/foundation.py, pcapkit/protocols/internet/ipv4.py:1212; and the swallowed Returns: at pcapkit/protocols/internet/ipv4.py:1550.
  • Malformed TCP SACK raises ProtocolError or FieldValueError depending on process state #525 records a genuine separate defect found here: which exception a malformed SACK produces is process-state dependent, ProtocolError from _read_mode_sack or FieldValueError from ListField.unpack, and they are siblings so no single except catches both. The test asserts the union, recording the problem rather than solving it. One correction to the original write-up: a deliberate attempt to force the FieldValueError path by running tests/protocols/schema/ first did not reproduce it, so the trigger is not pinned down and the test docstring no longer claims it is.

Tests

tests/test_docstring_contract.py                          5 passed / 10 subtests   exit 0
tests/protocols/transport/test_tcp_sack_length_unit.py    3 passed / 10 subtests   exit 0
tests/test_tier_guard.py                                 25 passed / 19 subtests   exit 0
tests/protocols/transport/ + schema unit                 81 passed / 73 subtests   exit 0

Both new files are unit tier by the guard's own path rule, read no capture under examples/captures/, and need no _tiers.py registration. test_docstring_contract.py imports no pcapkit at all — it reads source under its own root — so no editable install can shadow what it measures. Exit codes were read directly throughout, since pytest-subtests is not a dependency and pytest 9.1.1 prints a failing subtest's parent as PASSED.

One commit, rebased onto c8fd97bcd.

…#519)

Closes #519.

#501 fixed one wrong exception name and forty stale `Args:` labels by hand.
#519 showed the class was not exhausted. Re-deriving its census turned up one
finding that is not a docstring defect at all, plus a third class the issue
never mentions.

`TCP._read_mode_sack` documented `ProtocolError: If length is **NOT** multiply
of 8 plus 2` and never checked it. The tempting reading -- and the first one
taken here -- is that the clause was stale, like the other five phantoms. It is
the opposite, and three things settle it: RFC 2018 gives SACK a 2-octet header
followed by 8-octet edge pairs; both neighbouring readers validate their own
lengths, `_read_mode_sackpmt` on `!= 2` and `_read_mode_echo` on `!= 6`, with
the same message; and nothing upstream enforced it either, since the schema's
`ListField` consumes as many whole 8-octet items as it finds and ignores the
tail. So the docstring was right and the code was wrong.

- tcp.py: add the `(length - 2) % 8` check to `_read_mode_sack`, raising
  `ProtocolError` with the message its two siblings already use.
- httpv2.py: drop the `Raises: ProtocolError` clause from the five
  `_read_http_*` readers that cannot raise it. `_read_http_none` shows the
  mechanism -- its `raise` is commented out and replaced by a `ProtocolWarning`
  on the next line. All 11 `_read_http_*` methods carried the clause; the 6
  that genuinely raise keep it.
- tcp.py, twice: un-indent a `Returns:` header that sat inside the `Args:`
  block. This is the class the issue missed, and it loses documentation rather
  than merely misnaming it -- napoleon parses the header as a parameter, so the
  page rendered `:param Returns:` and carried no `:returns:` at all. Measured
  through `GoogleDocstring` directly, before and after.
- frame.py, schema/application/httpv2.py, schema/misc/pcapng.py: correct four
  `Args:` labels naming a parameter that does not exist. None of the four takes
  `**kwargs`, so each is a hard `TypeError`, not a cosmetic slip:
  `Frame.register(code=..., module=IPv4)` really does raise `TypeError: got an
  unexpected keyword argument 'module'`, while `protocol=` is accepted.
- hip.py: document the parameters `_make_param_reg_response` and
  `_make_param_route_dst` omit, in the wording their own siblings
  `_make_param_reg_failed` and `_make_param_route_via` already use.
- tests: `test_tcp_sack_length_unit.py` pins the length rule, accepted and
  rejected cases both. `test_docstring_contract.py` walks every function under
  `pcapkit/` and checks each documented name against the real signature, each
  documented exception against the real `raise` statements, and each section
  header against its indentation. It imports no pcapkit, reading source under
  its own root, so no editable install can shadow what it measures.

Census re-derived at this base rather than taken from the issue, which counted
against an older one:

    phantom Raises:                  6   -> 0 remaining
    Args: naming a missing parameter 13  -> 9 remaining, all allowlisted
    swallowed section headers        3   -> 1 remaining, allowlisted

The `Args:` figure is 13 here and was 14 when #519 was written: #511 retired the
IPX socket scrape and renamed that crawler's `soup` parameter to `data`,
incidentally fixing one. The nine that remain are in files owned by other
in-flight changes and are recorded in `KNOWN_DEFECTS` with a reason each, under
a test asserting every one still reproduces -- so the list cannot rot into a
description of bugs that are gone. That test has already earned its keep: it
failed on exactly the IPX entry during the rebase, which is how the stale entry
was found and removed.

Be clear about what is provable. A docstring label is not executed, so the
corrections cannot fail a test individually -- the contract test is what pins
them, by deriving the answer from the code instead of snapshotting today's
wrongness. Verified by reintroducing each class into a scratch tree: baseline
exit 0, phantom `Raises:` exit 1, bad `Args:` label exit 1, re-indented
`Returns:` exit 1. The two `Raises:` assertions are complementary rather than
redundant, and `_read_http_none` proves it -- `any(header.flags)` in its body
makes the conservative reachability check judge it possibly-raising, so only the
commented-out-raise check catches it. Measured: reachability exit 0, commented
raise exit 1.

The SACK change is the one with a real behavioural fails-before. Lengths 3, 11,
14, 17, 19 and 25 all parsed clean on `c8fd97bcd` with the SACK option present
in `tcp.info['options']`, and all six now raise `ProtocolError: TCP: [OptNo 5]
invalid format`.

Two things deliberately not done. All 12 functions carrying both `**kwargs` and
an `Args:` section without documenting `**kwargs` are in hip.py -- 438 of 450
document it elsewhere -- and that set includes the two siblings this change
copies its wording from, so fixing 4 of the 12 is what would make hip.py
inconsistent. And the SACK check implements exactly the documented rule, not
RFC 2018's one-to-four block bound; `length=2` is degenerate and still parses,
recorded in the test as a decision.

Tests: 5 passed / 10 subtests (contract), 3 passed / 10 subtests (SACK),
25 passed / 19 subtests (tier guard), 81 passed / 73 subtests (transport and
schema units, unchanged by the new check).

An independent second scanner, written from scratch against the same baseline,
reproduced all three counts (6 / 13 / 3) and the residuals (0 / 9 / 1), and
turned up three things now recorded in the test module:

- `_documented_names` justified its relative-indent measurement by saying
  `__doc__` is dedented at compile time. True of `__doc__` and irrelevant here,
  since this module reads `ast.get_docstring(..., clean=False)`, which preserves
  raw source indentation -- measured [0, 8, 12, 12] against [0, 0, 4, 4] for the
  dedented forms. The implementation was right for a different reason (nesting
  depth moves the absolute column); the rationale is corrected rather than left
  wrong in a checker for wrong rationales.
- `_raised_names` resolves only `Name` and `Attribute` raise targets, and
  `ast.walk` attributes a nested `def`'s raise to the enclosing function. Both
  can only miss a phantom, never fail a correct docstring, and both are now
  documented with their measurements: 856 `Name` + 2 `Attribute` + 0 `Subscript`
  across 858 raise targets, and 11 documented functions with a nested `def`, 3
  raising inside it. The second is the correct answer rather than a gap --
  `_read_param_locator_set` documents `ProtocolError` and raises none itself,
  but calls a `_read_locator` helper that does.
- One reported defect was a false positive and is recorded as a trap:
  `Raw.__post_init__` documents `error` and `alias`, has neither in its
  signature, and assigns a local `alias` -- but forwards `**kwargs` to `read`,
  which declares both as keyword-only and uses them. Reporting it would ask for
  correct documentation to be deleted.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Pushed an update (6bfdebe81b1124264e, one commit, same 8 files). No behavioural change; the SACK check and every docstring correction are byte-identical. What changed is tests/test_docstring_contract.py's own prose, after an independently written second scanner was run against the same baseline.

It reproduced all three counts (6 phantom Raises: / 13 Args: mismatches / 3 swallowed headers) and the residuals (0 / 9 / 1), including that the Args: figure is 13 here rather than the 14 counted against the older base. It also raised three things:

  1. A wrong rationale in the checker itself, now corrected. _documented_names justified measuring indentation relative to the section header by saying Python 3.13 dedents docstrings at compile time. That is true of __doc__ and irrelevant here — this module reads ast.get_docstring(node, clean=False), which preserves raw source indentation. Measured on 3.14.7 for a method whose Args: sits at eight spaces:

    get_docstring(clean=False)  ->  indents [0, 8, 12, 12]
    get_docstring(clean=True)   ->  indents [0, 0,  4,  4]
    compiled __doc__            ->  indents [0, 0,  4,  4]
    

    The implementation was right, for a different reason — nesting depth moves the absolute column, so a module-level function has Args: at 4 and entries at 8 where a method has 8 and 12. A wrong rationale inside a checker for wrong rationales is worth correcting explicitly rather than leaving.

  2. Two latent false-negative paths, now documented with measurements. _raised_names resolves only Name and Attribute raise targets (across pcapkit/: 856 Name, 2 Attribute, 0 Subscript — so currently inert), and ast.walk attributes a nested def's raise to the enclosing function (11 documented functions contain a nested def, 3 raise inside it). Both can only miss a phantom, never fail a correct docstring. The second turns out to be the correct answer rather than a gap: HIP._read_param_locator_set documents ProtocolError and raises none itself, but calls a _read_locator local helper that does, once per locator — so the exception genuinely propagates and the clause is not a phantom. It would only mislead for a nested def that is returned rather than called, which this package does not do.

  3. One reported defect was a false positive, and is now recorded as a trap rather than acted on. Raw.__post_init__ documents error and alias, has neither in its signature, and assigns a local alias from self._info.protocol.name — which reads exactly like a docstring naming a parameter that does not exist. It is not: __post_init__ forwards **kwargs to unpack, which dispatches to read, and read:66-67 declares both as keyword-only parameters and uses them at :81-82. The local merely shares a name with the keyword. Reporting it would have asked for correct documentation to be deleted — which is exactly what the **kwargs exclusion exists to prevent, so it earned a place in the module docstring.

Local: contract 5 passed / 10 subtests, SACK 3 passed / 10 subtests, tier guard 25 passed / 19 subtests, all exit 0.

@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE — the one behavioral fix (SACK length validation) independently fails-before/passes-after for all 6 invalid lengths with exit codes verified directly, the swallowed-Returns: claim is confirmed literally by feeding both the broken and fixed docstring text through napoleon's own parser, and every re-derived count (phantom Raises: 6, Args: mismatches 13-at-base, swallowed headers 3-at-base) matches the PR's own figures exactly once its 13-vs-14 explanation (an already-merged #511 incidentally fixed one in between) is accounted for.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Detailed review (independent verification, falsify-not-bless)

Reviewed at head b1124264e913e5d8d83e6f55947c6f359ae191c1 in an isolated worktree.

1. The one real behavioral fix — SACK length validation, independently reproduced. TCP._read_mode_sack gained if (schema.length - 2) % 8 != 0: raise ProtocolError(...) before constructing Data_SACK, making the method's own docstring (Raises: ProtocolError: If length is NOT multiply of 8 plus 2) true for the first time. Siblings _read_mode_sackpmt and _read_mode_echo already validated their own lengths, confirming this was the outlier.

  • Deleted just the 2-line check in my own worktree (reverting to main's behavior) and reran the new test: exit 1, 6 failed, 3 passed, explicit AssertionError: (ProtocolError, FieldValueError) not raised for all six invalid lengths (3, 11, 14, 17, 19, 25). Directly observed the pytest-9.1.1 mislabeling the PR warns about — the short summary printed PASSED for the parent test right next to six SUBFAILED(length=N) lines for the same test, confirming the instruction to check the real exit code rather than the summary.
  • Restored the fix, confirmed git diff --stat empty (back to the exact PR commit), reran: exit 0, all green.

2. Honesty of scope. The PR states outright that the docstring corrections "cannot fail a test individually" and separately claims real fails-before coverage only for the SACK check, with its own reproduction numbers. It does not imply broader behavioral coverage than it has. Confirmed honest.

3. Counts re-derived independently, then reconciled against the PR's own explanation.

  • Phantom Raises: — 6 → 0. 5 fixed by deleting stale clauses in httpv2.py (5 _read_http_* methods), 1 fixed by the SACK behavioral change making its existing clause true.
  • Args: naming a nonexistent parameter — 13 at base, 9 remaining (all in KNOWN_DEFECTS), 4 fixed. 13 (not the issue's "11", and initially appears to contradict "14") — but the PR description itself explains this precisely: 14 was the true count when issue #501 left behind: 6 phantom Raises: clauses and 11 Args: name mismatches #519 was filed (the issue undercounted by 3), and 13 is the count at this PR's actual base c8fd97bcd, because the already-merged fix(vendor): retire the IPX socket scrape, its data source is gone (#507) #511 incidentally fixed one Args: mismatch in between (IPX socket scrape's soupdata rename). Both figures are correct at their respective points in time; the PR discloses this itself rather than silently picking one. My independent count of 9 allowlisted entries + 4 diff fixes = 13 round-trips exactly.
  • Swallowed Returns: headers — 3 at base, 1 remaining (allowlisted), 2 fixed. Fixed: tcp.py:2979 and tcp.py:3006 (dedented from inside Args: to top-level Returns:). Not fixed, and not claimed to be fixed: ipv4.py:1550, explicitly named in the PR's own "Reported, not fixed (other streams own the files)" section and present in KNOWN_SWALLOWED. Confirmed the mis-indentation genuinely exists there on main/this branch (file untouched by this diff).
  • Napoleon verification, literal not inferred. Fed the actual mis-indented ipv4.py:_make_opt_e_sec docstring into sphinx.ext.napoleon.GoogleDocstring directly: renders :param Returns: Constructured option schema. with no :returns: field at all. Fed the fixed form (modeled on tcp.py's post-fix layout): renders correctly as :returns:. Confirms the "renders as a parameter named Returns" claim literally.

4. tests/test_docstring_contract.py — both properties confirmed.

5. #525 — confirmed present, not raised as a finding, per instruction. The union assertion (assertRaises((ProtocolError, FieldValueError))) exists exactly as described, with the test's own docstring recording that a deliberate attempt to force the FieldValueError path did not reproduce it. This is a disclosed, deliberately-unresolved state-dependent issue tracked separately — not treated as a gap in this PR.

What remains unverified

The broader tests/test_tier_guard.py and full tests/protocols/transport/+schema unit sweep numbers cited in the PR were not independently re-run (targeted tests directly falsifying the load-bearing claims were run and are clean; the broader counts are secondary and unverified by me, though I have no reason to doubt them given the targeted runs).

Verdict

All load-bearing claims independently reproduce, including a full, honest reconciliation of the 13-vs-14 Args: count. Recommend merge.

@JarryShaw
JarryShaw merged commit 48c6f07 into main Sep 20, 2026
12 of 24 checks passed
@JarryShaw
JarryShaw deleted the fix-519 branch September 20, 2026 05:24
JarryShaw added a commit that referenced this pull request Sep 20, 2026
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.
JarryShaw added a commit that referenced this pull request Sep 20, 2026
…#530) (#538)

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.
@JarryShaw JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

#501 left behind: 6 phantom Raises: clauses and 11 Args: name mismatches

1 participant