Skip to content

fix(vendor): send a descriptive User-Agent and retarget the dead pcapng draft - #524

Merged
JarryShaw merged 4 commits into
mainfrom
fix/518-vendor-ua-and-dead-urls
Sep 20, 2026
Merged

JarryShaw merged 4 commits into
mainfrom
fix/518-vendor-ua-and-dead-urls

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 19, 2026

Copy link
Copy Markdown
Owner

Closes #518.

What was broken

Four crawlers took a 403 on requests' default User-Agent — Wikipedia now requires a descriptive one — and three pointed at an ietf.org/staging URL that 404s.

The User-Agent

New get_user_agent() in pcapkit/vendor/default.py, built from importlib.metadata rather than hard-coded, applied at both the direct and the proxy call site. It is not a browser spoof: it names the distribution, version and repository, per Wikimedia's robot policy.

Proven metadata-driven rather than literal: faking the metadata to a different name and repository URL makes the function follow it, and the label match is case-insensitive.

The argument that the header cannot change generated output: of the 92 distinct crawler URLs, 87 return byte-identical bodies with and without it. The only five that differ are the four Wikipedia 403→200 and the one dead 404. Identical bodies imply identical output — stronger than running 117 generators twice.

The pcapng draft — both URLs the issue proposed are wrong

draft-tuexen-opsawg-pcapng-02 renders its registries as preformatted ASCII art: exactly one HTML table (the running header) and zero table-N ids, while all three crawlers select tables by id. So both -02 URLs fetch 200 and then raise IndexError. Measured by running the real BlockType.process() against the fetched page.

Only -03 reproduces all three constant files byte-identically. -04, -05 and ietf -00 diverge on block_type; ietf -01/-02 on all three; ietf -03 onward raise ValueError on a 0x0A0D0AXX wildcard row — so migrating to the renamed draft-ietf-opsawg-pcapng is real work, not a URL swap, and is left as a follow-up.

Chose the static www.ietf.org/archive/id/ rendering over datatracker: datatracker renders per request, sends no Last-Modified, and wraps the document in chrome adding three unrelated tables to a document the crawlers index tables out of.

ipx/packet.py — scrape retired

The table was deleted from the article on 2026-08-25 and the registry is closed, so DATA is hand-maintained with per-entry citations and the class defines no LINK. pcapkit/const/ipx/packet.py regenerates byte-identically (md5 0655b561ccdca4981ef6f7974ebeb66f).

ftp/return_code.py — a second defect, and why a data file changes

The article now ends its table with a cell-less <tr class="mw-empty-elt">, so indexing the first cell raised IndexError. That is why this constant file had never been regenerated. With the guard in place it regenerates — and gains CODE_335, RFC 2228's ADAT response. 53 → 54 members, nothing removed (verified with comm).

To be explicit, since a data change inside an infrastructure fix deserves it: this is not a hand-edit. Running the unmodified request()/count()/process()/context() pipeline against today's live page reproduces the committed file byte-for-byte, CODE_335 included. The only other differences are doc comments — the enum values for codes 450 and 452 are byte-identical either side.

Tests — 25 tests, 35 subtests, all three suites new

Mutation-tested by mutating files on disk with hash-checked restore. 14 of 14 mutations caught, including dropping the header from each call site independently.

Proven to fail without their fixes

revert result
FTP empty-row guard removed 4 failed, 1 passed — real IndexError at vendor/ftp/return_code.py:221
case-insensitive label match dropped 3 failedSUBFAILED on Repository, REPOSITORY, RePoSiToRy
as shipped 25 passed, 35 subtests

The one test that survives the guard removal is the fixture-shape assertion, correctly — it exists so that a fixture which stopped reproducing the defect would be caught, rather than silently making the others pass.

115 of 117 constant files regenerate byte-identically. The two exceptions are ipx.socket, whose crawler #511 retires, and mh.handover_initiate_status — see below.

Found while doing this, reported not fixed

pcapkit/const/mh/handover_initiate_status.py is the only one of 117 constant files missing the generated-file marker, and the cause is a duplicated export. MH_HandoverACKStatus appears twice in __all__ at line 196 of both pcapkit/vendor/mh/__init__.py and pcapkit/const/mh/__init__.py, and MH_HandoverInitiateStatus has no import line at all. Since pcapkit/vendor/__main__.py:87 builds its target list from __all__, that crawler has never run — 117 defined, 116 reachable. Not failing; unwired. Confirmed three independent ways. Its own file, so a separate change.

A vendor-reproduction CI gate is drafted but deliberately not included: two of its three steps fail today by design until that typo is fixed.

Also out of scope here: the dead docs URL still appears 15 times across 3 files, and #522 (the _request() fallback loop) was filed from a failure hit while testing this.

…ng draft

Closes #518.

- default.py: add get_user_agent(), built from importlib.metadata rather than
  hard-coded, and pass it at both the direct and the proxy call site. Wikipedia
  answers requests' default User-Agent with 403, which took out four crawlers.
  Not a browser spoof: it names the distribution, version and repository.
- pcapng/{block_type,option_type,record_type}.py: retarget from the dead
  ietf.org/staging draft to the -03 archive. The issue's proposed -02 URLs are
  both wrong -- that revision renders its registries as preformatted ASCII art
  with one table and no table-N ids, so the crawlers fetch 200 and then raise
  IndexError. Only -03 reproduces all three files; -04 onward diverge and the
  renamed draft-ietf-opsawg-pcapng raises on a 0x0A0D0AXX wildcard row.
- ipx/packet.py: retire the scrape. The table was deleted from the article on
  2026-08-25 and the registry is closed, so DATA is hand-maintained with
  per-entry citations and the class defines no LINK.
- ftp/return_code.py: skip cell-less rows. The article now ends its table with
  an empty <tr>, so line[0] raised IndexError -- which is why this constant file
  had never been regenerated, and why regenerating it now adds CODE_335
  (RFC 2228's ADAT response). 53 -> 54 members, nothing removed.

New tests/vendor/test_user_agent_unit.py, test_ipx_packet_unit.py and
test_ftp_return_code_unit.py: 25 tests, 35 subtests. 115 of 117 constant files
regenerate byte-identically; 87 of 92 response bodies are unchanged by the
header, the five that differ being the four 403s and the dead 404.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review: GOOD TO GO

Reviewed at head c41a56605 on a different model, briefed to falsify and to attack the two tests added after the earlier review, since nobody had adversarially read those. It ran four parallel subagents over disjoint file sets and confirmed no clobbering at the end (git status --short empty, HEAD unchanged).

Corrections to this PR's own text

  • The IndexError in the guard-removal repro fires at vendor/ftp/return_code.py:221, not 229. 229 is where the guard sits in the unmutated file; deleting its nine lines shifts everything up eight. Description now corrected.
  • The 53 → 54 diff is not purely a clean addition. Three pre-existing entries' doc comments also shifted — CODE_229's spacing, and the File unavailable (e.g., file busy). clause moving from CODE_452 to CODE_450. Still fully covered by the byte-identical regeneration proof and still nothing removed, but "growing from 53 to 54 with nothing removed" undersells that a few strings moved too.

Independently verified

Both new tests hold up, and the negative test genuinely constrains. The adversarial push was to mutate the label comparison to something looser rather than stricter — == changed to a substring test. The case-insensitivity test still passed superficially, and test_unrelated_project_url_labels_are_ignored failed, because 'repository-mirror' contains 'repository'. So the fix cannot be "loosen the comparison". Separately confirmed the lru_cache clearing is load-bearing rather than cosmetic: populating the cache with the real call, then mocking metadata without clearing, returns the stale real answer.

The FTP fixture genuinely reproduces the defect through the real parse path, re-parsed independently with the installed bs4 4.15.0 + html5lib 1.1: the trailing <tr class="mw-empty-elt"> really has zero <td>/<th>. And no network call — traced through cls.__new__, which never reaches _request(). One of the four failures fails one column over, on line[1] rather than line[0], which is exactly why the guard tests len(line) < 2 rather than not line.

-02 confirmed unusable: 1 <table> total, 0 ids matching table-N, and all 8 of the three crawlers' exact selectors return zero matches; calling .request() against the fetched page raises IndexError. -03 regenerates all three pcapng const files byte-for-byte. Bonus: -04 was fetched and shown to genuinely diverge on block_type via a renamed member (systemd_Journal_Export_Block..._I_D).

The FTP regeneration is mechanical, re-derived against the live page with no retries and no warnings: md5 fc9c72adbb1b4730d007ef31d35bcffd on both sides, and a comm over two independent extraction methods gives 53 → 54, one addition, zero removed.

Roughly 18 of the 115 constant files were independently regenerated and diffed byte-for-byte across the four agents — including 9 IANA CSV crawlers, reg.linktype from tcpdump.org and vlan.priority_level from live Wikipedia — all identical. Consistent with, though not exhaustive proof of, 115/117.

Both judgement calls endorsed: retiring a scrape against a closed registry trades no freshness for removed fragility, and bundling the regenerated constant file is right because CODE_335 is the observable proof the guard works — splitting it would leave the guard untestable and the data change unmotivated.

Not fully verified, stated plainly

beautifulsoup4[html5lib] is unpinned in pyproject.toml, so a future html5lib could in principle normalise away the empty <tr> the fixture depends on. Only 1.1 was tested. Low probability, not a blocker — but the fixture-shape assertion exists precisely to fail loudly if that ever happens.

Of the PR's claimed 14-of-14 caught mutations, only the 2 header-drop mutations were independently re-exercised; the other 12 were not reproduced.

One reviewer-side finding worth recording

One of the four subagents reported that the proxy call site lacks the header. That is wrong — line 489 reads requests.get(self.LINK, headers=headers, proxies=proxies) and git diff HEAD is empty. It was a transient read while a sibling agent was mid-mutation-test in a shared worktree; three of the four children observed the same transient state and attributed it correctly. Worth knowing as a hazard of parallel mutation testing rather than anything about this PR.

@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE — cross-reviewed clean; 87 of 92 crawler responses byte-identical with the new User-Agent, and the regenerated FTP constant file confirmed as mechanical crawler output rather than a hand-edit.

Comment thread pcapkit/vendor/pcapng/block_type.py
The reason all three scraping crawlers read draft revision -03 lived only in a
`#` comment block, and the docs pull in nothing but autoclass output, so Sphinx
never rendered any of it. Meanwhile the rendered pages still cited the dead -02
staging URL, so the docs pointed at a 404 while the code fetched -03.

- add a note to the pcapng vendor docs stating once that BlockType, OptionType
  and RecordType all read -03, that the -02 staging URL is dead, and that the
  table-1 through table-10 ids they select on first exist in -03
- retarget staging/draft-tuexen-opsawg-pcapng-02.html at
  archive/id/draft-tuexen-opsawg-pcapng-03.html in all 31 places that cited it
  across the vendor, const and protocols docs and sources; all 8 anchor
  fragments were confirmed present in -03
- leave 3 deliberately historical -02 references intact: the "which is dead"
  URL in block_type.py, and two draft-ietf-opsawg-pcapng-02 section citations
  in protocols/misc/pcapng.py that quote that revision's own text
- trim the block_type.py comment to the implementer-facing measurements, the
  reader-facing rationale now being in the rendered note

Sphinx 9.1.0 builds clean: 83 warnings, warning set byte-identical to the
parent commit, and no clickable -02 link left anywhere in the output. Refs #518.
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE — the .. note:: addition renders the pcapng -03 rationale that previously lived only in an invisible # comment, the 31 URL retargets are correct, and the two-draft-series distinction for the 4 references deliberately left on -02 is independently confirmed by fetching both actual IETF drafts: the quoted if_tzone/if_tsoffset deprecation language exists verbatim in draft-ietf-opsawg-pcapng-02 and does not exist at all in draft-tuexen-opsawg-pcapng-03.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Re-review of the delta (previous verdict covered c41a56605; this covers 1240093d2)

Isolating the real delta: git log --oneline c41a56605..1240093d2 shows one merge from main (absorbing 4 unrelated commits) plus one genuinely new commit, 1240093d2 docs: render the pcapng -03 rationale and retarget the dead -02 URL. Confirmed git diff c41a56605 23146419d touches nothing pcapng-related, so that one commit is the entire real delta: 8 files, 58 insertions, 46 deletions — matching the description.

The load-bearing claim — two distinct draft series — holds, verified against the actual documents, not just names. Fetched both drafts directly:

  • draft-ietf-opsawg-pcapng-02: authored by Tuexen et al., explicitly stated to replace draft-tuexen-opsawg-pcapng — same lineage, but a genuinely distinct, independently-numbered WG document once adopted (revision counter resets).
  • draft-tuexen-opsawg-pcapng-03: fetched directly and confirmed the quoted text does not exist there — if_tzone is a bare "TODO: specify better" stub with no deprecation language, and if_tsoffset is tied to if_tzone rather than divorced from local-time conversion (the opposite of what's quoted in pcapng.py). if_iana_tzname doesn't exist in tuexen-03 at all.
  • Re-fetched draft-ietf-opsawg-pcapng-02 to confirm exact wording: "it SHOULD NOT be used; instead, the if_iana_tzname option SHOULD be used" (if_tzone) and "This offset is not intended to be used as an offset between local time and UTC" (if_tsoffset) — verbatim matches to pcapkit/protocols/misc/pcapng.py:1240 and :1300. Bumping those citations to tuexen--03 would have been a factual error quoting language that document never contained.
  • The 4th reference left on -02 (beyond block_type.py's comment and the two pcapng.py citations the commit message calls "3") is the new .. note:: text itself, which necessarily names the dead -02 URL to explain why -03 was chosen — self-consistent with the commit's own count. grep -rn "pcapng-02" across pcapkit/ and docs/ → exactly 4 hits, matching this breakdown. A third-party corroboration: tests/protocols/misc/test_pcapng_unit.py:3088 independently cites the same draft-ietf-opsawg-pcapng-02 §4.2 language, untouched by either commit.

Anchor verification — all 8 confirmed, not just spot-checked. Fetched draft-tuexen-opsawg-pcapng-03's actual HTML and checked every anchor used in the diff: 7 section-heading anchors plus section-4.2-28.2.1 (used in filter_type.py's TODO) — all real and correctly targeted. One forward-looking note (not a defect): paragraph-level ids like that one are more fragile across future draft revisions than heading ids.

The .. note:: rendering fix. Confirmed the removed rationale previously lived only inside a #-prefixed source comment (not a docstring) preceding the class definition — invisible to Sphinx autodoc, which only pulls docstrings. RST syntax was verified statically (no Sphinx build available in the review environment — not installed, and standing one up was judged more than a "quick" check for this delta, so static analysis was used instead): correct .. note:: marker, indentation, and blank-line placement; backtick balance verified programmatically (46 backticks, all paired: 3 :class: roles + 10 double-backtick literals); py_compile succeeds on all 5 touched .py files.

Rest of the 8 files. Nothing beyond the note, the 31 URL retargets (exact count confirmed via diff), and the block_type.py comment trim — the file's other untouched paragraphs (archive-vs-datatracker byte-count comparison, the "-03 onward process() dies on 0x0A0D0AXX" note) were checked and retained verbatim.

What remains unverified

No actual Sphinx build was run against this delta specifically (relied on static RST/backtick analysis, which gives high confidence but isn't a substitute for a real render) — this is consistent with the previous review's build comparison, which covered the base commit this delta sits on top of, not this delta's own note addition in isolation.

Verdict

No regressions found in the delta; the one load-bearing judgment call (two-draft-series distinction) is independently confirmed against primary sources. Recommend merge.

@JarryShaw
JarryShaw merged commit a8912b4 into main Sep 20, 2026
23 checks passed
@JarryShaw
JarryShaw deleted the fix/518-vendor-ua-and-dead-urls branch September 20, 2026 05:21
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.

Vendor crawlers: three take a Wikipedia 403 on the default User-Agent, three point at a dead IETF URL

1 participant