Repository navigation
fix(vendor): make the handover_initiate_status crawler reachable (release blocker for make vendor) - #531
Conversation
`pcapkit/vendor/__main__.py` builds its crawler target list out of `__all__` rather than by discovery, at line 87 for a bare `pcapkit-vendor` and line 79 for `--target <name>`. Both MH lists named 'MH_HandoverACKStatus' twice and had no entry for 'MH_HandoverInitiateStatus', so 117 crawlers were defined and only 116 could ever run. - pcapkit/vendor/mh/__init__.py: import HandoverInitiateStatus, which was never imported into the package at all, and replace the duplicate `__all__` entry with it. The missing import is the second half of the defect: repairing `__all__` alone would have raised AttributeError. - pcapkit/vendor/__init__.py: replace the same duplicate `__all__` entry. No import needed -- it wildcard-imports pcapkit.vendor.mh. - Both docstring tables listed MH_HandoverACKStatus against the text "Handover Initiate Status Codes"; corrected to match the footnotes. - docs/source/pcapkit/const/mh.rst: the same displaced row, so the .rst agrees with pcapkit/const/mh/__init__.py as corrected by #527. - tests/vendor/test_crawler_reachability_unit.py: assert every Vendor subclass under pcapkit/vendor/** is reachable from both target-list paths, and that no `__all__` repeats an entry. The constant file is deliberately not regenerated; that needs network access and belongs to the pre-release `make vendor` run. isort and mypy clean; pylint reports no message the two files did not already report on main. 5 new tests pass, 4 of them fail without the fix.
The first commit fixed this row in `pcapkit/vendor/mh/__init__.py`'s docstring table, which left `docs/source/pcapkit/vendor/mh.rst` disagreeing with it by one row -- the same divergence in the vendor tree that the `const/mh.rst` change in that commit closes in the const tree. Line 53 named MH_HandoverACKStatus against the text "Handover Initiate Status Codes", so MH_HandoverACKStatus appeared twice and MH_HandoverInitiateStatus not at all. The footnote list underneath was already ordered #handover-initiate-status then #handover-acknowledge-status, so correcting the row makes the table agree with its own footnotes. The per-crawler body section further down the file was always correct -- it already carries `.. autoclass:: pcapkit.vendor.mh.handover_initiate_status.HandoverInitiateStatus` -- so only the summary table needed the change.
|
✅ GOOD TO MERGE — the crawler is proven reachable end-to-end (hand-rolled the exact |
Detailed review (independent verification, falsify-not-bless)Reviewed across both commits: 1. The fix itself, confirmed by reading the diff. 2. End-to-end reachability — proven independently of both the diff and the shipped test. The exact mechanism 3. The new test ( 4. Fails-without-fix / passes-with-fix, reproduced with exit codes captured directly (not through a pipe). Reverted to 5. No regressions. No code anywhere depends on the duplicate ( 6. 7. Falsification — "listed in 8. Disjointness from #527 — confirmed zero file overlap ( Additional checks beyond the brief: The follow-up commit (
|
Release blocker for the
make vendorrun before 1.5.0Read this part first.
pcapkit/vendor/__main__.pydoes not discover crawlers — itbuilds its target list out of
__all__, at line 87 for a barepcapkit-vendorandline 79 for
--target <name>:Both MH lists named
'MH_HandoverACKStatus'twice and had no entry for'MH_HandoverInitiateStatus'. So 117 crawlers are defined underpcapkit/vendor/**and only 116 could ever run, measured on unfixed
main:make vendorispipenv run pcapkit-vendorwith no arguments — exactly the line 87path — so the intended pre-1.5.0 regeneration would silently skip this one crawler
and ship a stale enum, with no warning, no non-zero exit, and nothing in the log to
notice.
.github/workflows/cron-vendor.ymlruns the same bare command, which is whythe crawler has never executed on any schedule either.
The corroborating evidence is in the file the crawler owns.
Vendor.context()emits a:meta private:directive into every enumeration it writes(
pcapkit/vendor/default.py:70). 116 of the 117 files underpcapkit/const/*/carryit. The one that does not is
pcapkit/const/mh/handover_initiate_status.py— theoutput of the one crawler that could not be reached. It also still uses the older
two-line
extend_enum(...); return cls(value)form that 86 of its siblings have sincebeen regenerated away from. The duplicate dates to
24edd30d2(2023-04-17, "added MHenumerations & vendor cralwers"), so the file has been stale for the crawler's whole
life.
The missing import, not just the missing
__all__entryChecked rather than assumed, and the answer is yes, the import was missing too — the
same second half the
constpackage had:pcapkit/vendor/mh/__init__.pywent straight fromhandover_initiate_flagtohome_address_reply;handover_initiate_statuswas never imported into the package atall. Repairing
__all__alone would have turned a silently-skipped crawler into anAttributeErroron star-import. So this PR adds the import and fixes the entry.pcapkit/vendor/__init__.pyneeds no import — it wildcard-importspcapkit.vendor.mh,so the name arrives once
mhexports it. Only its__all__entry changes.What changed
pcapkit/vendor/mh/__init__.py— importHandoverInitiateStatus as MH_HandoverInitiateStatus(wrapped, so the line stays underisort -l100); replacethe duplicate
__all__entry with it; and fix the docstring table row, which namedMH_HandoverACKStatusagainst the text "Handover Initiate Status Codes". Thefootnote list underneath was always in the right order (
#handover-initiate-statusthen
#handover-acknowledge-status) — only the row was wrong, so correcting it makesthe table and its footnotes agree again.
pcapkit/vendor/__init__.py— the same duplicate__all__entry, replaced.docs/source/pcapkit/const/mh.rst— the second item. fix(protocols): repair __all__ entries that name nothing, and guard the class #527 corrected this exactdisplaced row in
pcapkit/const/mh/__init__.py's docstring but could not touch the.rst, so the two disagreed by one row. The change here is character-identical tofix(protocols): repair __all__ entries that name nothing, and guard the class #527's, modulo the
const/vendorprefix, so the.rstand the module docstringagree once both land.
docs/source/pcapkit/vendor/mh.rst— the same displaced row in the vendor docspage, in the second commit. Correcting the
pcapkit/vendor/mh/__init__.pydocstringabove would otherwise have created this divergence rather than closed it — the same
defect one file to the left. The table and the module docstring are now byte-identical
on both rows, verified by diff. The per-crawler body section further down was always
correct: it already carries
.. autoclass:: pcapkit.vendor.mh.handover_initiate_status.HandoverInitiateStatus, so only the summarytable needed changing.
tests/vendor/test_crawler_reachability_unit.py— new, 5 tests.Not regenerated, deliberately
pcapkit/const/mh/handover_initiate_status.pywill change on the nextmake vendor, and this PR does not touch it. Regenerating needs network access to theIANA registry, and it belongs to the owner's pre-release run rather than to a fix that
only makes the crawler reachable. Verifying that the regeneration now happens is the
point at which this change proves itself.
The test: a reachability contract, not a pin
The defect is a displaced duplicate, and that shape is invisible to every obvious
assertion: each
__all__entry still resolves (the name it repeats is real), everymodule still imports, the class still exists on disk, and the list is still the right
length. Only comparing the crawlers that are defined against the crawlers that are
reachable finds it — so that is what the test asserts, over all 117 rather than as a
pin on one:
test_every_crawler_is_reachable_from_the_default_target_listpcapkit-vendor, i.e.make vendorand the crontest_every_crawler_is_reachable_from_its_own_protocol_target--target mh, a separate list that was separately wrongtest_the_two_target_lists_agreetest_no_duplicate_all_entriestest_handover_initiate_status_is_reachableWritten this way it also catches the next crawler added to a subpackage without
being listed — the same mistake, and an easy one, since adding a crawler means editing
two
__all__lists in two files that nothing checked against the filesystem.Crawler classes are only ever looked at, never constructed.
Vendor.__init__fetches from IANA and writes the constant file as a side effect ofconstruction, so instantiating one in a test would make a network call and edit the
working tree.
Why a new file rather than extending #527's
#527 added
tests/project/test_public_api.py, which sweeps__all__across the tree anddeliberately excludes
pcapkit.vendorviaEXCLUDED_ROOTS— it is unit-tier and thevendorextra is not inpip install -e '.[test]'. Its docstring records theremediation as "drop
'vendor'fromEXCLUDED_ROOTS". I went with my own file for tworeasons: #527 is not merged, so that file does not exist on the branch this is cut
from; and the assertion here is a different one — defined-versus-reachable is about the
crawler registry, not about
__all__hygiene. Dropping the exclusion is still worthdoing on top of this and would give the duplicate a second, cheaper detector; it is
left to #527's own follow-up rather than done here, since that file is not mine to edit.
The
vendorextra is present in this environment (bs44.15.0,requests2.34.2), sothe suite runs rather than skipping — but it is guarded with
skipUnlessthe same waytests/vendor/test_ipx_socket_unit.pyguards the identical two dependencies, so anenvironment without them skips instead of erroring.
Fails without the fix
Reverting only the two source files and keeping the test:
The assertions, verbatim:
Restoring both files:
One test passed in the red run, and that is worth stating rather than glossing:
test_the_two_target_lists_agreepassed against the unfixed tree, because both listswere wrong in the same way and therefore agreed. It is a drift guard for the root
__all__falling out of step with its subpackages', not a detector of this defect — so4 of the 5 are the fails-without evidence, not all 5.
Adjacent suites, after the fix:
pytest tests/vendor tests/const tests/project -q→25 passed, 127 subtests passed, exit 0. The full suite and
coveragewere not run —seven other agents were on this host and it OOM-killed three processes earlier today.
isort -l100 -ppcapkitclean on bothpcapkit/vendor/mh/__init__.pyand the new test.mypyclean.pylintwith the Makefile's flags reports the same message set beforeand after on the two changed modules (
C0301:mh/__init__.py:135,R0801duplicate-code,and three command-line meta messages — all pre-existing), so this adds no new finding.
pcapkit/vendor/__init__.py's import block is not isort-ordered onmaineither; it isexcluded by the Makefile's
--skip-glob '**/__init__.py', and this PR does not touch it.Merge order does not matter
Worth stating because the branch looks inconsistent in isolation: on this branch
pcapkit/const/mh/__init__.pystill carries the duplicate, because that is #527's halfand #527 is unmerged. So
docs/source/pcapkit/const/mh.rsthere is the forward half of apair whose other half lands separately. That is safe in either merge order — the
:class:role's target,pcapkit.const.mh.handover_initiate_status.HandoverInitiateStatus,is a class that exists on disk regardless of any
__all__, and the text before the anglebrackets is display text rather than a resolution target. Nothing in the suite compares
.rsttables against module docstrings, so neither order can break a build.Reported, not fixed
No GitHub issue tracks this defect. #530 is a real issue about an unrelated RFC
citation in
hopopt.py, so nothing here cites an issue number — an early draft of thetest did, wrongly, and every invented reference was stripped before the first commit.
Worth filing one for the record if the history matters later.
The constant file is still not regenerated, deliberately, as above: that needs network
access to the IANA registry and belongs to the pre-release
make vendorrun.