Skip to content

fix(vendor): make the handover_initiate_status crawler reachable (release blocker for make vendor) - #531

Merged
JarryShaw merged 3 commits into
mainfrom
fix/vendor-mh-handover-initiate-unreachable
Sep 20, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
fix/vendor-mh-handover-initiate-unreachable

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 20, 2026 •

Copy link
Copy Markdown
Owner

Release blocker for the make vendor run before 1.5.0

Read this part first. pcapkit/vendor/__main__.py does not discover crawlers — it
builds its target list out of __all__, at line 87 for a bare pcapkit-vendor and
line 79 for --target <name>:

# pcapkit/vendor/__main__.py:79   — the `--target <name>` path
target_list.extend(getattr(module, name) for name in module.__all__)
# pcapkit/vendor/__main__.py:87   — the default path, i.e. a bare `pcapkit-vendor`
target_list.extend(getattr(vendor_module, name) for name in vendor_module.__all__)

Both MH lists named 'MH_HandoverACKStatus' twice and had no entry for
'MH_HandoverInitiateStatus'. So 117 crawlers are defined under pcapkit/vendor/**
and only 116 could ever run
, measured on unfixed main:

crawlers defined: 117
__all__ len: 117 unique: 116
reachable via pcapkit.vendor.__all__: 116
UNREACHABLE: ['pcapkit.vendor.mh.handover_initiate_status.HandoverInitiateStatus']

make vendor is pipenv run pcapkit-vendor with no arguments — exactly the line 87
path — 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.yml runs the same bare command, which is why
the 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 under pcapkit/const/*/ carry
it. The one that does not is pcapkit/const/mh/handover_initiate_status.py — the
output 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 since
been regenerated away from. The duplicate dates to 24edd30d2 (2023-04-17, "added MH
enumerations & vendor cralwers"), so the file has been stale for the crawler's whole
life.

The missing import, not just the missing __all__ entry

Checked rather than assumed, and the answer is yes, the import was missing too — the
same second half the const package had:

mh has MH_HandoverInitiateStatus attr: False
vendor has MH_HandoverInitiateStatus attr: False

pcapkit/vendor/mh/__init__.py went straight from handover_initiate_flag to
home_address_reply; handover_initiate_status was never imported into the package at
all. Repairing __all__ alone would have turned a silently-skipped crawler into an
AttributeError on star-import. So this PR adds the import and fixes the entry.

pcapkit/vendor/__init__.py needs no import — it wildcard-imports pcapkit.vendor.mh,
so the name arrives once mh exports it. Only its __all__ entry changes.

What changed

  • pcapkit/vendor/mh/__init__.py — import HandoverInitiateStatus as MH_HandoverInitiateStatus (wrapped, so the line stays under isort -l100); replace
    the duplicate __all__ entry with it; and fix the docstring table row, which named
    MH_HandoverACKStatus against the text "Handover Initiate Status Codes". The
    footnote list underneath was always in the right order (#handover-initiate-status
    then #handover-acknowledge-status) — only the row was wrong, so correcting it makes
    the 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 exact
    displaced 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 to
    fix(protocols): repair __all__ entries that name nothing, and guard the class #527's, modulo the const/vendor prefix, so the .rst and the module docstring
    agree once both land.
  • docs/source/pcapkit/vendor/mh.rst — the same displaced row in the vendor docs
    page, in the second commit. Correcting the pcapkit/vendor/mh/__init__.py docstring
    above 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 summary
    table needed changing.
  • tests/vendor/test_crawler_reachability_unit.py — new, 5 tests.

Not regenerated, deliberately

pcapkit/const/mh/handover_initiate_status.py will change on the next
make vendor, and this PR does not touch it. Regenerating needs network access to the
IANA 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), every
module 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 what it covers
test_every_crawler_is_reachable_from_the_default_target_list the line 87 path — bare pcapkit-vendor, i.e. make vendor and the cron
test_every_crawler_is_reachable_from_its_own_protocol_target the line 79 path — --target mh, a separate list that was separately wrong
test_the_two_target_lists_agree the root re-lists its subpackages by hand; two copies of one fact
test_no_duplicate_all_entries the mechanism, asserted directly
test_handover_initiate_status_is_reachable this crawler by name, at all three levels

Written 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 of
construction, 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 and
deliberately excludes pcapkit.vendor via EXCLUDED_ROOTS — it is unit-tier and the
vendor extra is not in pip install -e '.[test]'. Its docstring records the
remediation as "drop 'vendor' from EXCLUDED_ROOTS". I went with my own file for two
reasons: #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 worth
doing 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 vendor extra is present in this environment (bs4 4.15.0, requests 2.34.2), so
the suite runs rather than skipping — but it is guarded with skipUnless the same way
tests/vendor/test_ipx_socket_unit.py guards the identical two dependencies, so an
environment without them skips instead of erroring.

Fails without the fix

Reverting only the two source files and keeping the test:

$ git checkout origin/main -- pcapkit/vendor/__init__.py pcapkit/vendor/mh/__init__.py
$ pytest tests/vendor/test_crawler_reachability_unit.py -q
4 failed, 1 passed          # exit code 1
FAILED ...::test_every_crawler_is_reachable_from_the_default_target_list
FAILED ...::test_every_crawler_is_reachable_from_its_own_protocol_target
FAILED ...::test_handover_initiate_status_is_reachable
FAILED ...::test_no_duplicate_all_entries

The assertions, verbatim:

AssertionError: Lists differ: ['pcapkit.vendor.mh.handover_initiate_status.HandoverInitiateStatus'] != []
AssertionError: Lists differ: ['pcapkit.vendor.mh.handover_initiate_status.HandoverInitiateStatus'] != []
AssertionError: 'MH_HandoverInitiateStatus' not found in ['MH_Packet', 'MH_Option', ...]
AssertionError: {'pcapkit.vendor': ['MH_HandoverACKStatus'], 'pcapkit.vendor.mh': ['MH_HandoverACKStatus']} != {}

Restoring both files:

$ git checkout HEAD -- pcapkit/vendor/__init__.py pcapkit/vendor/mh/__init__.py
$ pytest tests/vendor/test_crawler_reachability_unit.py -q
5 passed                    # exit code 0

One test passed in the red run, and that is worth stating rather than glossing:
test_the_two_target_lists_agree passed against the unfixed tree, because both lists
were 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 — so
4 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 coverage were not run —
seven other agents were on this host and it OOM-killed three processes earlier today.

isort -l100 -ppcapkit clean on both pcapkit/vendor/mh/__init__.py and the new test.
mypy clean. pylint with the Makefile's flags reports the same message set before
and after on the two changed modules (C0301:mh/__init__.py:135, R0801 duplicate-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 on main either; it is
excluded 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__.py still carries the duplicate, because that is #527's half
and #527 is unmerged. So docs/source/pcapkit/const/mh.rst here is the forward half of a
pair 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 angle
brackets is display text rather than a resolution target. Nothing in the suite compares
.rst tables 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 the
test 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 vendor run.

`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.
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE — the crawler is proven reachable end-to-end (hand-rolled the exact getattr(module, name) for name in module.__all__ mechanism from vendor/__main__.py against the live package: 117 defined, 117 reachable, 0 unreachable, in both the root and --target mh paths), the "listed but not imported" falsification is caught hard by Python's own wildcard-import semantics before any test assertion even runs, and the follow-up commit (07fa02e1e, correcting the same displaced row in docs/source/pcapkit/vendor/mh.rst) is independently verified correct — its footnote ordering (handover-initiate-status at line 624, before handover-acknowledge-status at 625) matches the corrected table row exactly.

@JarryShaw

Copy link
Copy Markdown
Owner Author

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

Reviewed across both commits: 3a5db947b6eb8012e1a76d119bae14df43ea5e5d (the substantive fix) and 07fa02e1eb906c30e9084232e3cf44b57d48b051 (a follow-up docs correction pushed after the first review pass — checked separately below).

1. The fix itself, confirmed by reading the diff. pcapkit/vendor/mh/__init__.py imports HandoverInitiateStatus as MH_HandoverInitiateStatus from pcapkit.vendor.mh.handover_initiate_status, lists it exactly once in __all__, and the duplicate MH_HandoverACKStatus entry is replaced (not just deleted) by the correct name. pcapkit/vendor/__init__.py gets the identical __all__ substitution.

2. End-to-end reachability — proven independently of both the diff and the shipped test. The exact mechanism pcapkit/vendor/__main__.py uses (getattr(module, name) for name in module.__all__, lines 79/87) was hand-rolled against the live imported package: root pcapkit.vendor.__all__ path — 117 entries, 117 unique, MH_HandoverInitiateStatus present, resolves to the real Vendor subclass; --target mh subpackage path — 42 entries, same clean resolution. A full pkgutil.walk_packages defined-vs-reachable scan: 117 defined, 117 reachable, 0 unreachable, 0 extra.

3. The new test (tests/vendor/test_crawler_reachability_unit.py, 5 tests) genuinely exercises the mechanism — discovers crawlers via filesystem truth (pkgutil.walk_packages) and resolves reachability via getattr exactly as __main__.py does, rather than a weaker "string present in __all__" check. Crawler classes are only inspected via isinstance/issubclass, never constructed, correctly avoiding the IANA network call side effect of Vendor.__init__.

4. Fails-without-fix / passes-with-fix, reproduced with exit codes captured directly (not through a pipe). Reverted to main: 4 failed, 1 passed, exit 1 — the one pass (test_the_two_target_lists_agree) is correct because both lists were wrong identically, matching the PR's own honest caveat. Restored: 5 passed, exit 0.

5. No regressions. No code anywhere depends on the duplicate (__all__.count(...) etc. — none found). The analogous duplicate in pcapkit/const/mh/__init__.py is confirmed to be #527's scope (already reviewed, disjoint file set, zero overlap with this PR), not something #531 introduces or misses. pytest tests/vendor tests/const tests/project -q → 25 passed, 127 subtests, exit 0.

6. docs/source/pcapkit/const/mh.rst — table row correctly repointed; parsed with bare docutils, confirmed the changed row produces the same (expected, Sphinx-vs-bare-docutils) error class as every other unchanged row — no new/structural error.

7. Falsification — "listed in __all__ but not imported." Caught hard: because pcapkit/vendor/__init__.py does from pcapkit.vendor.mh import *, Python's own wildcard-import semantics raise AttributeError the moment pcapkit.vendor is imported (which happens in every test's setUp()) — before any of the test's own assertions run. A second, narrower falsification not in the original checklist — a phantom name added to the root __all__ that resolves to nothing — slips past all 5 shipped tests (since getattr(..., None) silently filters it from "reachable," and it was never "defined" either). This is a real, narrow gap, but it's independently caught by the project's own pylint --enable=undefined-all-variable gate (E0603: Undefined variable name ... in __all__) — not a blocker.

8. Disjointness from #527 — confirmed zero file overlap (const/* vs vendor/*).

Additional checks beyond the brief: isort, and mypy/pylint run with the project's actual Makefile flags and scope — both clean, pylint score unchanged at 9.73/10 with an identical message set to main. A mypy union-attr nit exists in the new test file, but the project's make mypy target never checks tests/, so it's outside actual CI scope — noted, not a blocker.

The follow-up commit (07fa02e1e), verified separately

This PR moved one commit past what the sub-review checked. Fetched and inspected it directly: a single-line fix to docs/source/pcapkit/vendor/mh.rst, correcting the exact same displaced-row defect (MH_HandoverACKStatus mislabeling the "Handover Initiate Status Codes" row, MH_HandoverInitiateStatus appearing nowhere) that the first commit already fixed in const/mh.rst's equivalent table. Verified independently: the footnote list underneath the table has #handover-initiate-status at line 624 and #handover-acknowledge-status at line 625 — the corrected row order (MH_HandoverInitiateStatus row before MH_HandoverACKStatus row) now matches that footnote sequence exactly. The per-crawler .. autoclass:: body section further down the file was already correct and untouched, consistent with the commit message's claim that only the summary table needed the change.

Verdict

Both commits verified. The crawler reachability mechanism is proven end-to-end, not just by string presence, and the trailing docs commit is a correct, consistent fix. Recommend merge.

@JarryShaw
JarryShaw merged commit 739068a into main Sep 20, 2026
24 checks passed
@JarryShaw
JarryShaw deleted the fix/vendor-mh-handover-initiate-unreachable branch September 20, 2026 05:22
@JarryShaw JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Sep 22, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 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

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant