fix(ngap): source ProcedureCode/ProtocolIE from pycrate, stop the shared -1 mint (#880)

[JarryShaw]

Please follow the guide below

• 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 -- N/A -- changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• fix — corrects a defect

Description of your pull request and other information

Completes #880's ngap.py half (the mh.py half landed in #889). Per the owner's ruling, ProcedureCode/ProtocolIE are genuinely open per 3GPP TS 38.413, so they want a per-key unique value rather than the shared -1 sentinel #880 measured aliasing on -- sourced from pycrate's own compiled NGAP specification through a vendor crawler, not hand-maintained or minted on demand.

• New pcapkit/vendor/ngap/{procedure_code,protocol_ie}.py: crawlers whose _request() imports pycrate_asn1dir.NGAP directly (no network fetch -- pycrate is already an installed optional dependency) and reads NGAP_Constants' (name, value) pairs, filtered by each value's own typeref.
• New pcapkit/const/ngap/{procedure_code,protocol_ie}.py, generated: EnumRegistry + IntEnum, matching the house pattern. get() no longer mints on an unresolvable string key; _missing_ answers an unseen in-range value with a throwaway, non-registering member instead of a shared-value extend_enum mint -- which is what stops two different unknown keys from aliasing.
• pcapkit/protocols/application/ngap.py now re-exports both under their historical names, dropping ~600 lines of hand-rolled enum + extend_enum logic.
• Four tests/const/ generic sweeps' pinned counts move by +2 (two more IntEnum registries under pcapkit.const); tests/const/test_const_registry_protocol.py's own census is out of scope for this PR (another PR owns it) and needs 122->124, 105->107.

New tests in tests/vendor/ and tests/protocols/application/, each shown to fail without this fix.

[JarryShaw]

This PR breaks a test it correctly does not touch, and the fix has to land inside it. Verified by direct count rather than taken on trust:

const .py files (excl __init__)   origin/main: 122    this PR: 124

tests/const/test_const_registry_protocol.py:852-853 asserts len(all_files) == 122 and len(generated) == 105. The two new generated modules make that 124 / 107, and running that one test on this branch fails with AssertionError: 124 != 122.

The author was right to leave the file alone — I had declared it off-limits. But the bump cannot go to main separately either: only 122 files exist there, so pushing 124 ahead of this PR breaks main immediately. So it belongs in this PR, and I have handed the file to this branch and retracted it from the other agent that was told it might own it. Four sibling sweeps with the same +2 shape (test_const_enum_builtin_parity.py, test_const_enum_get.py, test_const_enum_lookup.py) were already updated here.

Two corrections to my own brief, both found by the author and both confirmed:

1. I attributed the "pycrate-sourced crawler" ruling to fix: six .get implementations mint unknown keys at a shared -1, so the second aliases to the first #880. It is not there — grep -ci pycrate returns 0 against this issue's body and 0 against its comments. The framing is in PR fix(mh): stop the four RFC-inline helper enums from minting on unassigned bytes #889's body: "ruled to be sourced from pycrate through a vendor crawler instead (separate work), so fix: six .get implementations mint unknown keys at a shared -1, so the second aliases to the first #880 stays open." Consistent with fix: six .get implementations mint unknown keys at a shared -1, so the second aliases to the first #880's own "genuinely open per 3GPP TS 38.413" language, but I quoted the wrong source and should not have.
2. I described pcapkit/const/ngap/ and pcapkit/vendor/ngap/ as existing trees to be modified. Neither existed — git ls-tree origin/main returns 0 files for each. ProcedureCode and ProtocolIE were hand-pasted IntEnum classes inside ngap.py. Both trees were built from scratch, which is a larger job than I briefed.

One thing I want the review to look at, since it changes the public surface: ProcedureCode and ProtocolIE in ngap.py are now plain module-level aliases (ProcedureCode = Enum_ProcedureCode) rather than subclasses, because an aenum enum with members cannot be subclassed. Aliasing preserves pickle resolution — pickle's find_class does a plain getattr — but issubclass and identity checks against the old classes behave differently from before. Whether that warrants breaking is the open question.

Head is now 94fd58e6b after a rebase onto current main; the cross-review has been re-pointed at it.

[JarryShaw]

NEEDS CHANGES on 94fd58e6b — cross-review (opus; author sonnet). Two items. The second is new and appears nowhere in the PR, and it is the one that would have shipped unnoticed.

1. The census delta, and a correction to my own framing. The counts are confirmed 122 → 124 and 105 → 107, derived from test_const_registry_protocol.py:845-853 (rglob over pcapkit/const, __init__.py excluded) and reproduced as a live failure on this head: AssertionError: 124 != 122, Ran 74 tests ... FAILED (failures=1). Also checked: the two new files are subjected to the generated list's per-file body assertions and pass all of them, so the count bump is the entire delta — no exception-set entry needed. But I was wrong to call this undeclared — this PR's own body, line 19, already says "needs 122->124, 105->107". It is a declared sequencing hazard, not a silent break. Handled: I have given this branch the file (see my comment above).

2. NEW — this permanently breaks the cron-vendor exit code. Verified independently:

pycrate is deliberately excluded from the all extra. pyproject.toml says why, in its own words: "Size. pycrate ships every specification it has ever compiled in one distribution: installing it lands ~238 MB … to obtain the one 4.9 MB NGAP.py this needs … a 50x multiplier" and "Licence. pycrate is LGPL-2.1+ where this package is BSD-3-Clause." Resolved, all is ['emoji', 'cryptography>=3.4', 'dpkt', 'scapy', 'pyshark', 'pypcapfile…', 'requests[socks]', 'beautifulsoup4[html5lib]'] — no pycrate.

But .github/workflows/cron-vendor.yml:114 installs -e .[all] and :131 runs a bare pcapkit-vendor, capturing the exit code at :132. Bare pcapkit-vendor collects from pcapkit.vendor.__all__, which now contains both new crawlers. With no pycrate on that runner both _request() calls raise ImportError, __main__.py returns False per target, and main() returns 1.

So from the moment this merges, every scheduled vendor run emits two ::warning title=pcapkit-vendor target failed:: annotations and the reported failing target(s) summary — permanently. Nothing is corrupted (_snapshot_and_restore protects the const files and the job is not hard-failed), but the signal is destroyed: #872 added that non-zero exit specifically so a no-op regeneration is distinguishable from a real one, and this makes it non-zero forever for a reason that is never a real failure.

Fix: add pycrate to the cron-vendor install step. It belongs in this PR — the change that creates the dependency should keep the scheduled run meaningful. Excluding the new crawlers from the default target list is the wrong alternative, since it contradicts test_crawler_reachability_unit.py's contract.

Everything else was confirmed and much of it strongly. Byte-exact offline reproducibility (589e6a15… and f389d902…, run1 == run2 == committed); the crawler genuinely derives from NGAP_Constants._all_ filtered on _typeref.called rather than transcribing; the module header is .. module:: pcapkit.const.ngap.procedure_code, not __main__; an AST diff shows 81/81 and 438/438 name-and-value identity with nothing dropped, renamed or duplicated; the mint is gone with Cls(200) and Cls(201) now distinct and non-registering; pickle round-trips and preserves identity through the module-level alias; and all four modified test files are genuine strengthenings — notably nothing was added to EXPECTED_TO_RESOLVE_ANYTHING or its sibling exception sets, which was the weakening move available here.

One thing for you rather than the author: unknown-string get now raises instead of returning a per-key value. The ruling asked for "a per-key unique value rather than the shared -1", which holds for unknown integers but not for unknown strings — where there is no value to assign. Defensible and explicit in the PR, but not literally what was ruled, so confirm rather than assume.

[JarryShaw]

GOOD TO GO on f41de64fb — both NEEDS CHANGES items fixed, verified by me rather than taken from the hand-back.

The amend touched exactly what it should and nothing else. git diff --stat 94fd58e6b f41de64fb:

.github/workflows/cron-vendor.yml           | 15 ++++++++++++++
tests/const/test_const_registry_protocol.py | 31 +++++++++++++++++++----------
2 files changed, 35 insertions(+), 11 deletions(-)

Still exactly one commit above main. So everything the cross-review confirmed on 94fd58e6b — the byte-exact offline reproducibility, the AST-verified 81/81 and 438/438 member identity, the mint being gone, pickle round-tripping through the module-level alias, and all four test files being genuine strengthenings — carries forward untouched.

Census (item 1) — fixed properly, not just the two numbers. :861-862 now assert 124 and 107. Ran the whole file at this head in a tree I asserted:

TREE /tmp/v899/pcapkit/__init__.py
Ran 74 tests in 70.498s
OK

The three remaining occurrences of 105 in that file are explanatory prose about the transition — "105 is what is left once the 6…", "105 itself does not move there", "6 and 11 hold; 105 becomes 107, and 122…" — not stale live counts. The docstring gained a third historical paragraph rather than overwriting the existing 121→122 narrative, and five other live mentions of "the 105 default-template registries" were updated to 107. CONVERTED (6) and EXCLUDED_FROM_INTENUM_CENSUS (11) correctly untouched, since the two new files are the plain generated shape and join neither set.

cron-vendor (item 2) — fixed, and the comment earns its place. python -m pip install -U pycrate added after the [all] install, with a comment that states the tension directly rather than leaving a future reader to wonder why CI installs what pyproject.toml deliberately excludes: the ~238 MB / 50x size multiplier, the LGPL-2.1+ against BSD-3-Clause licence asymmetry, and why this job differs from an end user's [all] — bare pcapkit-vendor regenerates every crawler including the two NGAP ones, which read pycrate_asn1dir.NGAP directly. Confirmed independently that cron-vendor.yml is the only workflow invoking pcapkit-vendor, so nothing else needed the same treatment, and the default-target-list contract test_crawler_reachability_unit.py depends on was left alone.

One question still open for you, not a blocker. Unknown-string get now raises KeyError rather than returning a per-key value. Your #880 ruling asked for "a per-key unique value rather than the shared -1", which holds exactly for unknown integers — Cls(200) and Cls(201) are now distinct and non-registering — but for an unknown string there is no value to assign, and minting one is the defect being fixed. Defensible and explicit in the PR body, and the reviewer did not block on it. Worth a word if you read it differently.

Ready for you to merge once CI finishes on this head. I am not merging.

[JarryShaw]

We should add pycreate to [vendor]extras as well in this case.

[JarryShaw]

Ruling recorded, verbatim (reading pycreate as pycrate):

We should add pycreate to [vendor]extras as well in this case.

Agreed, and it is a better fix than what shipped — the current commit adds a bare python -m pip install -U pycrate to cron-vendor.yml, which puts the dependency in a workflow rather than in the metadata that declares what running the crawlers needs. vendor is already commented # for developers at pyproject.toml:203, and the NGAP crawlers are exactly that: maintainer tooling. So the declaration belongs there and the workflow should just ask for the extra.

One detail decides whether this alone is enough, and it is not. all duplicates vendor's contents inline rather than referencing the extra:

vendor = [ "requests[socks]", "beautifulsoup4[html5lib]" ]              # :203
all    = [ …, "requests[socks]", "beautifulsoup4[html5lib]", ]          # :217-227

and all carries a deliberate comment: "pycrate is left out for the size and licence reasons set out on the NGAP extra above; pip install pypcapkit[NGAP] is the way to it."

So adding pycrate to vendor does not put it in all, and cron-vendor.yml:114 installs -e .[all]. The workflow must install .[vendor] as well or it stays broken — the bare pip install pycrate gets replaced by -e .[all] -e .[vendor], not simply deleted. That all comment stays true and needs no edit, since all still will not carry it.

A consequence worth stating rather than discovering later: pip install pypcapkit[vendor] will now pull ~238 MB of pycrate_asn1dir and an LGPL-2.1+ dependency into a BSD-3-Clause package. That is defensible for a developer-facing extra in a way it was not for all — which is precisely the distinction your ruling draws — but the vendor extra currently has no comment saying so, while NGAP and all both explain themselves at length. It gets one, so the next reader does not think it was an accident.

Sent to the author. Note this will move the head, so the review: good-to-go currently on f41de64fb gets re-pinned rather than carried — a verdict on a superseded head is worse than none.

[JarryShaw]

GOOD TO GO re-pinned to 846dd0058 — your [vendor] ruling is implemented correctly. Verified myself rather than from the hand-back.

delta since f41de64fb:  cron-vendor.yml 25 +-,  pyproject.toml 12 +-,  tests/test_tier_guard.py 13 +-
                        still exactly one commit
vendor  = ['requests[socks]', 'beautifulsoup4[html5lib]', 'pycrate']       :211
NGAP    = ['pycrate']                                                     unchanged
all has pycrate?  False                                                   <- the thing that must not regress
cron-vendor.yml:122  python -m pip install -e .[all,vendor]               bare `pip install -U pycrate` gone

So the dependency is declared where running the crawlers is declared, all is untouched along with its own comment, and the workflow asks for the extra instead of reaching past it. That is the shape your ruling described.

It also found the test I asked it to look for, which I would not have predicted: tests/test_tier_guard.py::DependencyRequirementTests::test_a_bracket_inside_a_requirement_does_not_end_the_array pinned vendor's requirement-name set to {'requests', 'beautifulsoup4'}. Updated to include pycrate with the docstring extended. The adjacent assertion that all excludes pycrate is still correct and was left alone — which is the right call, since that one is the invariant your ruling deliberately preserves. 107 tests in that file pass under plain unittest.

One prose defect in the new comment, and it is the wrong way round on the very thing the comment exists to explain. pyproject.toml:202-210 ends:

a developer opting into vendor has already accepted a far heavier and more permissively-licensed dependency set than an end user installing all ever would.

LGPL-2.1+ is less permissive than BSD-3-Clause, not more — that is the entire reason pycrate was kept out of all. As written the sentence argues the opposite of the decision it is justifying. The rest of the comment is accurate and genuinely useful (it names the ~238 MB, the licence pair, and attributes the ruling to you).

Not blocking on a comment, so this stays GOOD TO GO — but it should be corrected before or just after merge, because a comment that inverts a licence argument is worse than no comment. Your call whether to merge now and let me fix it, or have the author amend first; say which and I will act.

[JarryShaw]

Should pycrate (and crypto libraries) added to the all extra as well? I think that extra might need some revisit to determine what should fit in and what not - it's originally for all deps (when we only had 3 3p engines and crawler deps). Now that the library grew, a new audit on it should be done.

[JarryShaw]

Question recorded, verbatim:

Should pycrate (and crypto libraries) added to the all extra as well? I think that extra might need some revisit to determine what should fit in and what not - it's originally for all deps (when we only had 3 3p engines and crawler deps). Now that the library grew, a new audit on it should be done.

Half of it is already done: cryptography>=3.4 is fully in all, via the crypto extra. Measured on origin/main:

ALL       crypto       ['cryptography>=3.4']
ALL       DPKT / PyPCAPFile / PyShark / Scapy / cli / vendor
EXCLUDED  NGAP         ['pycrate']                  size (~238 MB) + LGPL-2.1+ vs BSD-3-Clause
EXCLUDED  PCAP_CT      ['pcap-ct', 'libpcap']       pre-release only
EXCLUDED  PyPCAP       ['pypcap']                   sdist-only C extension, needs a compiler
EXCLUDED  docs         [Sphinx, furo, ...]
partial   test         only requests/beautifulsoup4 reach `all`

So crypto needs no change. pycrate is the live question, and I would leave it out — the reasons in pyproject.toml's own comment still hold, and the sharpest one is that pip install pypcapkit[all] would then pull an LGPL-2.1+ dependency silently, which is precisely the hazard that comment names. Your [vendor] ruling already gets it to the people who need it, and they opt in knowingly.

You are right that the extra needs an audit, and the measurement shows why more concretely than I expected: there are three exclusions resting on three different reasons, and no rule that predicts any of them. Size, licence, release maturity and build-toolchain requirements are each argued locally in a comment, at length, and nothing says which of them disqualify an extra in general. So a contributor adding the next extra has nothing to check against — which is how this drifted in the first place.

Filed as #910, scoped to produce the criterion rather than just a revised list, because a list without a rule drifts again. It also has to reconcile two things the measurement turned up:

• all duplicates other extras' contents inline rather than referencing them. requests[socks]/beautifulsoup4[html5lib] are literals in both vendor and all, so the two drift independently — exactly what bit this PR, where adding pycrate to vendor did not put it in all and cron-vendor.yml had to ask for .[all,vendor].
• test is partial, which is almost certainly correct but is currently an accident of which names happen to overlap, not a decision.

This PR needs no change for any of it and its review: good-to-go stands: it only touched vendor, and all still excludes pycrate as intended — verified, all has pycrate? False.

Still outstanding on this PR, unchanged and not blocking: the new vendor comment at pyproject.toml:202-210 says a developer accepts a "more permissively-licensed" dependency set, where LGPL-2.1+ is less permissive than BSD-3-Clause. Merge and I fix it, or have the author amend first — your call.