Skip to content

fix(vendor): 16 crawlers' process() reads its own data argument (#2429) - #2430

Merged
JarryShaw merged 1 commit into
mainfrom
fix-vendor-process-data-2429
Oct 29, 2026
Merged

fix(vendor): 16 crawlers' process() reads its own data argument (#2429)#2430
JarryShaw merged 1 commit into
mainfrom
fix-vendor-process-data-2429

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 29, 2026

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort)
  • make test passes, and a test case covers the change
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

16 of the 24 dict-based pcapkit.vendor crawlers' process(self, data) ignored the data
argument and read the module-level DATA literal instead. Harmless today, since every real
call site passes request()'s own DATA-returning value, so data is DATA always held in
practice — but a caller passing different data would have gotten the module's table back
regardless, with no error.

pcapkit-vendor regenerates pcapkit/const/, not pcapkit/vendor/ itself, so these
hand-written crawlers needed no template fix and no const/ regeneration follows.

New test (tests/vendor/test_vendor_process_uses_passed_data_2429_unit.py) drives each
crawler's count()/process() on a fabricated registry and proves the output now reflects
the argument rather than the module global; confirmed to fail against the unfixed code first.

Closes #2429.

@github-actions github-actions Bot added this to the PyPCAPKit v1.5 milestone Oct 29, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 29, 2026
@JarryShaw JarryShaw added python Pull requests that update Python code fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 29, 2026
@github-actions

github-actions Bot commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Coverage: 94.44% (unit tier, Python 3.15, fd12a4c0a, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19609 133 2620 120 98.84%
pcapkit/corekit 2605 74 818 37 96.52%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 3064 24 1068 26 98.79%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17593 193 4678 179 98.32%
pcapkit/toolkit 924 36 334 10 95.55%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4449 1854 1020 167 55.11%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 29, 2026
@JarryShaw

JarryShaw commented Oct 29, 2026

Copy link
Copy Markdown
Owner Author

Review: NEEDS CHANGES at b2e69d9ec (Sonnet). Code fix itself is correct — every factual claim held, 101 of 118 subtests fail with exactly the 16 sources reverted. Two text-only blockers:

  1. Counted numbers in prose, violating the standing no-counted-numbers convention (ruled 2026-10-15, docs/source/contributing/conventions/documentation.rst:228; 1.5.0 is unreleased so this applies): "16 of the 24 dict-based crawlers" in CHANGELOG.md/1.5.0.rst, and in the new test's docstring/comments. The file lists already enumerate the modules — the count adds nothing. Same pattern test(hip): drop counted numbers from the #2394 str-name test #2402 fixed for fix(hip): resolve a str enum name in list-argument makers too #2401.
  2. The test docstring says the fabricated mapping "shares the module's codes" but the actual fabricated codes (900001/900002) are described two lines later as "well outside any real range" — contradicts itself.

Non-blocking: test_process_does_not_fall_back_to_the_module_table doesn't actually catch ipv4/tos_ecn (swapping vendor.record between its two calls changes output on its own, independent of the fix) — the sibling test still catches it, so not blocking, but worth tightening. Also: the test only covers the 16 hard-coded modules, so a future crawler could regress unnoticed — acceptable for this PR's scope.

Everything else independently re-verified: AST walk over all 97 process(self, data) methods confirms exactly 16 affected (matching the PR/issue list exactly), confirmed no vendor template writes to pcapkit/vendor/ (only pcapkit/const/ is generated), confirmed harmless-until-now (data is DATA held on every real path, rendered const text byte-identical base vs head), full tests/vendor/const/registry/template suites green, mypy/pylint/isort identical to base.

@JarryShaw
fix(vendor): 16 crawlers' process() reads its own data argument (#2429)…
fd12a4c 
… (#2430)

16 of the 24 dict-based crawlers' `process(self, data)` ignored the `data`
argument and read the module-level `DATA` literal instead: `ipv4/qs_function`,
`tos_del`, `tos_ecn`, `tos_pre`, `tos_rel`, `tos_thr`, `ts_flag`;
`ipv6/option_action`, `qs_function`, `smf_dpd_mode`; `mh/binding_error`,
`lla_code`; and `pcapng/filter_type`, `hash_algorithm`, `secrets_type`,
`verdict_type`. 8 siblings, such as `ipv4/classification_level`, already use
`data` correctly.

Harmless today, since every real call site passes `request()`'s own
`DATA`-returning value, so `data is DATA` always held in practice -- but a
caller passing different data, such as a test fixture or a future
`LINK`-based fetch (the open TODO in `pcapng/filter_type.py` already points
that direction), got the module's table back regardless, with no error.

`pcapkit-vendor` regenerates `pcapkit/const/`, not `pcapkit/vendor/` itself:
these dict-based crawlers are hand-written, so no template needed the same
fix and no `const/` regeneration follows from it.

New test drives each crawler's `count()` and `process()` on a fabricated
registry sharing none of the module's names, proving the rendered output now
reflects the argument rather than the module global; confirmed to fail
against the unfixed crawlers first.

Closes #2429.
@JarryShaw
JarryShaw force-pushed the fix-vendor-process-data-2429 branch from b2e69d9 to fd12a4c Compare October 29, 2026 05:12
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 29, 2026
@JarryShaw

JarryShaw commented Oct 29, 2026

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at fd12a4c0a. Verified directly (main thread). Both text-only fixes confirmed: counted numbers removed from CHANGELOG.md/1.5.0.rst/the test file (checked directly — no "16 of"/"of the 24" remains), and the module docstring now correctly says the fabricated codes are "disjoint from the module's own DATA" instead of the self-contradicting "shares the module's codes".

tests/vendor/ suite: 190 passed, 2209 subtests. isort clean; mypy's 13 errors are entirely in tests/_support.py, confirmed pre-existing on main. Code fix content unchanged from the prior round's independent verification.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 29, 2026
@JarryShaw
JarryShaw merged commit d951177 into main Oct 29, 2026
65 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 29, 2026
@JarryShaw
JarryShaw deleted the fix-vendor-process-data-2429 branch October 29, 2026 12:47
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 29, 2026
This was referenced Oct 29, 2026
Closed
Merged
JarryShaw added a commit that referenced this pull request Oct 29, 2026
@JarryShaw
docs: #1103 round 10 -- close the whole-repo prose sweep (#2432)
c2f1089 
Round 10, the closing-condition test: a full re-grep across the entire repo
found exactly 3 items, all outside the carve-outs round 9 already settled.

* pcapkit/protocols/protocol.py + foundation/registry/protocols.py:
  RegistryError said "protocol must be a Protocol subclass" while the
  adjacent issubclass() check and the Raises: docstring both say
  ProtocolBase -- fixed the message text to match, with the three test
  assertions that locked the old string.
* tests/foundation/reassembly/test_deferred_pickle_2423_unit.py: a docstring
  said "this pull request's earlier commit", stale now that #2424 landed as
  one squashed commit on main -- reworded to cite the issue instead of the
  PR's own history.
* tests/protocols/transport/test_tcp_mptcp_option_attr_2293_unit.py: a
  cross-reference cited the nonexistent module
  pcapkit.corekit.fields.enums.EnumField; the real path is
  pcapkit.corekit.fields.numbers.EnumField.

No executable logic changed: an AST structural diff (ignoring string
constants) against origin/main is identical for every touched file, and the
RegistryError fix only changes the text of a raised message, not what
triggers it.

Re-verified and rejected two relayed findings that didn't survive a direct
check: a transport.py:188-189 and a registry/protocols.py:370 cross-reference
claim both already read correctly on this base. Re-swept vendor/process()
for siblings of #2429/#2430 -- none found. docs/source and the rest of
tests/ came back clean.

Build: tests/protocols/test_register_class_guard_unit.py,
tests/foundation/registry/test_protocols.py,
tests/foundation/reassembly/test_deferred_pickle_2423_unit.py,
tests/protocols/transport/test_tcp_mptcp_option_attr_2293_unit.py and
tests/project/test_docs_issue_pr_roles_1103_unit.py pass.

Closes #1103.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

fix Pull requests that fix a defect (fix: subject prefix) python Pull requests that update Python code

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.5

Development

Successfully merging this pull request may close these issues.

fix(vendor): 16 crawlers' process() ignores its own data argument

1 participant

@JarryShaw