fix(vendor): stop get() minting; _missing_ fixed for 21 of 105 (#775) - #838
Conversation
JarryShaw
added
fix
48e02dc to
d40eeb1
Compare
|
Verified the tier 1 implementation myself before any verdict. Head is now Shape: one commit, author and committer Verified on
One thing I checked because the diff does not make it obvious: the new A full cross-review is running on a different model from the author, briefed to attack the parts my checks did not cover — all 105 files' |
|
NEEDS CHANGES — cross-review (opus, a different model from the sonnet author) on B1 — a real in-tree caller now raises where it used to return. Neither behaviour is good — on base every unresolvable name collapsed to one junk member at value B2 — the body's headline claim is false for most files it edits. It says an unregistered value/key "now stays absent from By Two corrections to numbers I had relayed. The split is 21/84, not 22/83 (49 sites is right, and replaying the substitution on each base
Dispatching the fix. Also taking the reviewer's aligned-signature point: |
JarryShaw
added
review: needs-changes
|
CI root cause on
I swept all 95
The two site families need different fixes. The toolkit pair gets Redirected the fix worker accordingly. |
d40eeb1 to
364c8d8
Compare
JarryShaw
added
review: pending
JarryShaw
changed the title
|
Fixes pushed at B1 — all six affected call sites fixed, with two different strategies. The toolkit pair passes an explicit default: B2 — title and body rewritten. The title is now Signature aligned: Verified by me on this head: all 115 files compile clean under One correction to something I asserted earlier, and it was my error. I had said the PR body needed "22/83 → 21/84" and the "49 sites" figure corrected. Neither ever appeared in the body — both live only in the cross-review comment above, which had already self-corrected. One thing I am not yet asserting:
|
|
NEEDS CHANGES — re-review (opus) on B1 — "an unresolvable wire value is a protocol error" is false, and the wire path still mints. The four guarded lookups read a So the B2 — the ~1007 is attributed to the wrong bucket. By B3 — Worse, the reviewer found the pyshark test double hides the common case: In fairness it does not regress something that worked — the old path minted The ask: for an unresolvable link-layer name at those two toolkit sites, do you want (1) Held and confirmed: the |
JarryShaw
added
review: needs-changes
364c8d8 to
53d8ed3
Compare
|
The unambiguous findings are fixed at B1 fixed. The four NOTEs now read that only a hand-constructed schema reaches the guard and that an unassigned wire value still mints through The robustness fix went further than I asked, and it was right to. My snippet bound the key outside the One nit I am not sending back: the missing-key message reports A number I published was wrong, and this corrects it. I relayed that the 781 deferred 766 of 781 (98%) are Still |
Not sure I followed but my idea is that, if it is a unregistered value but legit according to the Enum's range, then we are good to return a temporary enum using something like
Same, not entirely sure if I understood. I used But also, I'm not sure how industry handle these. Should we actually not to mint
Good catch! I say we raise, if |
|
Recorded, verbatim: "I say we raise, if B1 — your mechanism is already what the PR implements, so nothing is in dispute. "return a temporary enum using something like B2 — your instinct matches CPython exactly. Measured on this repo's 3.14.7: So no, do not mint B3 — your premise checks out, and there is a consequence worth naming. The consequence: scapy is fine — |
JarryShaw
removed
the
needs: decision
|
Can check documentations for the PyShark case if you cannot have tshark available/installed on the host to test live. And let's add the handlers (protocols) for DLT_NULL and DLT_RAW as part of wave 2/3 work in the work tracker then. |
|
Recorded, verbatim: "Can check documentations for the PyShark case if you cannot have tshark available/installed on the host to test live. And let's add the handlers (protocols) for DLT_NULL and DLT_RAW as part of wave 2/3 work in the work tracker then." Checked the docs and the installed source, and the
That makes the consequence concrete rather than speculative: Two items filed:
Neither blocks #838: raising is right with or without the handlers. The worker applying the ruling is instructed to correct the double to |
Tier 1 of #775: get()'s string-key miss no longer permanently registers a member, across all 105 edited pcapkit.const registries. _missing_'s bounded-range fix only changes behaviour for the 21 of those 105 whose vendor crawler leaves process() unmodified -- the other 84 keep their own bespoke process()-driven _missing_, which still mints via extend_enum, same as before. - pcapkit/vendor/default.py: get() raises (or falls back to a value lookup) instead of extend_enum()-ing a name; the 21-registry _missing_ range branch mints via a fresh _unregistered_member() instead. Both gain register(value, name), the explicit caller-named path that still grows the registry -- value-first, matching _unregistered_member's own order, so the two no longer disagree. - pcapkit/toolkit/scapy.py, pyshark.py: raise instead of falling back to LinkType.NULL in Enum_LinkType.get() -- per the #838 ruling, NULL and RAW are genuine DLTs with their own handler protocol classes, not stand-ins for "unknown link type". The bare KeyError is caught and re-raised as MissingKeyError, this package's own house exception for a lookup miss. A real caller -- an IP-rooted scapy packet, or any pyshark capture, since PyShark's layer name is Wireshark's PDML filter name (e.g. `eth`, not `ethernet`) and matches no LinkType member -- now raises instead of getting a minted placeholder; #840 tracks the name -> DLT mapping pyshark tracing will need. - pcapkit/protocols/internet/hopopt.py, ipv6_opts.py: the SeedID/TaggerID get() misses in _read_opt_mpl/_read_opt_smf_dpd now raise this reader's own ProtocolError instead of a bare KeyError, matching the neighbouring "unknown QS function"-style wording. The guard is for an unresolvable string key from a hand-built schema, not a wire value -- an unassigned wire value still mints via _missing_, deferred to tier 2. The key is pre-bound to None so the except handler can report it even when the namespace dict itself lacks the key, instead of re-subscripting and letting a second, bare KeyError escape. - 105 generated pcapkit/const/ modules hand-edited to match; proved equivalent by rendering the template per class and diffing get/register/_unregistered_member via ast.get_source_segment. - tests/const/test_const_enum_no_mint.py, tests/toolkit/test_scapy_unit.py, tests/toolkit/test_pyshark_unit.py and tests/protocols/internet/ test_ipv6_extension_unit.py: new/updated coverage for all of the above. Out of scope: the 12 wholesale-template vendor files' own _missing_ (781 extend_enum call sites), AppType.register_alias, and the other 84 registries' process()-driven _missing_ overrides (226 more) -- 1007 extend_enum call sites in total, deferred to tier 2. Build: py_compile clean on all 114 touched files. Tests: coverage run -m pytest tests/const/, 98 passed, 39458 subtests, 57.77% branch (57.79% baseline, steady); combined with the new/updated test paths above, 61.76%.
53d8ed3 to
dd9eee8
Compare
Tier 1 of #775: get()'s string-key miss no longer permanently registers a member, across all 105 edited pcapkit.const registries. _missing_'s bounded-range fix only changes behaviour for the 21 of those 105 whose vendor crawler leaves process() unmodified -- of the remaining 84, 83 keep their own bespoke process()-driven _missing_ and still mint via extend_enum, and the 84th has no unassigned range to mint from at all. - pcapkit/vendor/default.py: get() raises (or falls back to a value lookup) instead of extend_enum()-ing a name; the 21-registry _missing_ range branch mints via a fresh _unregistered_member() instead. Both gain register(value, name), the explicit caller-named path that still grows the registry -- value-first, matching _unregistered_member's own order, so the two no longer disagree. - pcapkit/toolkit/scapy.py, pyshark.py: let an unresolvable link-layer name raise. On base these sites minted: Enum_LinkType.get() defaulted to -1, so get('ETH') ran extend_enum(LinkType, 'ETH', -1) and returned a freshly minted LinkType.ETH = -1, and a second unresolvable name aliased onto the same -1 member. Asked to pick a real DLT as the fallback instead, the ruling was to raise: NULL and RAW are genuine DLTs with their own handler protocol classes, not stand-ins for "unknown link type". The bare KeyError is caught and re-raised as MissingKeyError, this package's own house exception for a lookup miss. An IP-rooted scapy packet now raises rather than getting a minted placeholder. - pcapkit/toolkit/pyshark.py: PyShark reports Wireshark's PDML filter name, not a DLT name -- `eth`, not `ethernet` -- so every Ethernet capture would otherwise raise, and engines/pyshark.py calls tcp_traceflow for each TCP packet under trace=True. A curated FILTER_NAME_TO_LINKTYPE table translates the two names that are unambiguous against Wireshark's dissector registrations -- `eth` -> ETHERNET and `tr` -> IEEE802_5, each bound to exactly one WTAP_ENCAP_* -- and is consulted before LinkType.get, which still resolves a filter name that already spells a member (`ppp`, `fddi`). A genuinely unknown name still raises. Measured with tshark 4.6.9 and editcap -T across the 158 of its 226 encapsulations an Ethernet source can be rewritten into: `sll` serves both LINUX_SLL (113) and LINUX_SLL2 (276) under one filter name, so it gets no entry and raises. `raw` serves RAW/IPV4/IPV6 (101/228/229) and `null` serves NULL/LOOP (0/108); neither raises, because 'RAW' and 'NULL' are member names, so the fallback answers 101 and 0 -- silently wrong for 228, 229 and 108, which is #843 rather than anything this table introduces. `ip` and `ipv6` never arrive as the root layer in any of those 158; a raw IPv6 capture roots at `raw`. The other 68 refuse the rewrite and are untested. `fr` turns out to be single-DLT (both its encapsulations write 107) so it is mappable, left out as unneeded scope; `wlan` was not investigated. Refs #840, which also asks for `ip` -> IPV4 and `loop` -> NULL. Only `ip` is refused; `loop` already resolves to LinkType.LOOP (108) through the fallback, and is anyway the wrong filter name -- tshark registers `loop` as Configuration Test Protocol (loopback), an Ethernet payload, while a DLT_NULL or DLT_LOOP capture presents `null`. `null` resolves to LinkType.NULL (0), so DLT_LOOP silently reads as DLT_NULL; that ambiguity is #843. The issue stays open. - pcapkit/protocols/internet/hopopt.py, ipv6_opts.py: the SeedID/TaggerID get() misses in _read_opt_mpl/_read_opt_smf_dpd now raise this reader's own ProtocolError instead of a bare KeyError, matching the neighbouring "unknown QS function"-style wording. The guard is for an unresolvable string key from a hand-built schema, not a wire value -- an unassigned wire value still mints via _missing_, deferred to tier 2. The key is pre-bound to None so the except handler can report it even when the namespace dict itself lacks the key, instead of re-subscripting and letting a second, bare KeyError escape. - The non-minting pseudo-member now carries the bare registry word -- Unassigned, not Unassigned_39 -- per the maintainer's ruling. The numeric suffix existed to keep a *minted* member unique in __members__; nothing is minted here, and a pseudo-member is absent from __members__, _member_map_ and _value2member_map_ alike, so two same-named ones coexist and repr still disambiguates by value. The 49 call sites across those 21 registries drop the suffix; every extend_enum name is left numbered, because a minted Unassigned would collide. - 105 generated pcapkit/const/ modules hand-edited to match, then proved equivalent by regenerating all 21 changed registries from live IANA: the resulting diff is byte-identical to the hand-edit, and seven further registries across both template kinds regenerate with no change at all, so no upstream drift is riding along. - tests/const/test_const_enum_no_mint.py, tests/toolkit/test_scapy_unit.py, tests/toolkit/test_pyshark_unit.py and tests/protocols/internet/ test_ipv6_extension_unit.py: new/updated coverage for all of the above. Out of scope, counted exactly by AST census rather than estimated: 1026 minting call sites remain -- 1007 in _missing_ and 7 in get() across pcapkit/const/, plus 12 hand-written ones in protocols/internet/mh.py and protocols/application/ngap.py that no regeneration can reach. AppType alone holds 767 of them. register()/register_alias account for a further 106 sites which are the by-design caller-named mint path and stay. Deferred to tier 2. Build: py_compile clean on all 117 touched files. Tests: coverage run -m pytest tests/const/, 98 passed, 39458 subtests, 57.77% branch (57.79% baseline, steady); combined with the new/updated test paths above, 61.76%.
1840f9a to
f5d4bda
Compare
|
Fifth round, three blockers, all mine again — head is now The substantive one. I claimed a sweep of "all 158 encapsulations That matters beyond the count, exactly as the review said: Second: the test file contradicted the comment this round had just fixed. The double's stub Third: a stale PR-body paragraph still said And the review turned up something that improves the table rather than just correcting it: 22 targeted tests pass; 100 across the wider selection on the prior head. One limit the reviewer hit and named, which I cannot close on this host: |
|
GOOD TO GO on What it re-derived rather than accepted: It also confirmed the One discrepancy it flagged instead of assuming, and it was right: my test count. It measured 91 + 9 = 100, 1 skip. The count was right and my brief was incomplete; nothing regressed. Two nits I am not revising for: of the 158 swept, 9 yield no determinable root and 30 root at Label flipped to Still outstanding on your |
JarryShaw
added
review: good-to-go
JarryShaw
removed
the
review: good-to-go
get()'s string-key miss and every generated registry's _missing_ bounded-range branch both called aenum.extend_enum() on an unrecognised value, so e.g. Hardware(40) minted a permanent member on first call, forever. Fixed at the two shared sites in pcapkit/vendor/default.py, plus a new register(value, name) classmethod for the caller-named path that is still meant to mint. All 105 generated pcapkit/const/ registries are hand-edited to match and proved by regenerating the 21 changed ones from live IANA data -- byte-identical. get()'s fix applies to all 105 registries; the _missing_ fix only changes runtime behaviour for the 21 whose vendor crawler leaves Vendor.process() unmodified. Two toolkit call sites and four protocol reader sites now raise MissingKeyError/ProtocolError instead of getting a minted placeholder back. 1026 minting call sites remain out of scope, deferred to a later tier.
…ap_type pcapkit/toolkit/pyshark.py resolved a frame's link type from packet.layers[0].layer_name, a Wireshark display-filter name, with a blanket Enum_LinkType.get(name.upper()) fallback. One filter name serves several encapsulations, so that fallback returned wrong DLTs silently -- null for a DLT_LOOP capture, 101 (RAW) for rawip6. Re-keyed onto frame.encap_type, which is distinct per encapsulation, and the fallback is gone: an unrecognised encapsulation or filter name now raises MissingKeyError. Both lookup tables were measured through editcap/tshark round trips rather than transcribed from Wireshark source, which is not on the build host: ENCAP_TYPE_TO_LINKTYPE grows to 152 entries and FILTER_NAME_TO_LINKTYPE grows from #838's 2 entries to 58.
Adds util/pyshark_encap_map.py, a generator that regenerates ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built stop being hand-maintained. Guards against its own worst failure mode: a naive LinkType(dlt) lookup cannot detect an unmapped DLT, because _missing_ mints a placeholder rather than raising, so the generator snapshots every known value before any lookup. The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is version-pinned -- editcap -T accepts 226 encapsulations on Wireshark 4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run only under the measured version. One line of pyshark.py itself changes as a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical name #848 gave value 209. util/changelog_md.py regenerated CHANGELOG.md for all seven entries added across this and the six preceding commits (#838, #846, #847, #848, #849, #850, #853); `--check` exit 0.
#846 and #850 - #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified" measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an unmodified process(), but its FLAG-bound range has no unassigned gap, so its _missing_ carries no bounded-range extend_enum branch to fix. The real discriminator is that branch, not the process() override; reworded, and the "other 84" split into the 83 that do override process() and the one that doesn't but has nothing to fix either. Re-derived independently against 5e25db2^..5e25db2 (not main, which would fold in #847's own edit to ipx/socket.py): 22 registries have no process() override, 21 of them have the bounded-range branch, matching tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining the exclusion). - #846's "Only tests/integration/test_engine_runtime.py and test_engine_parity.py change" is false -- 30bca99's own numstat also touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only these two test files change". - #850 gets the breaking marker: its own prose already says the fallback is gone and an unrecognised encapsulation or filter name now raises MissingKeyError instead of substituting a plausible DLT -- the same shape as the two existing markers at AppType.get (1.5.0.rst:2456) and the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to lead with the marker and the subject, matching their wording and placement; the later restatement of the same fact is dropped. None of these are code or test changes -- prose only, matching the cross-review's own framing. #848 stays unmarked (its own "Not breaking" paragraph holds); #838, #849 and #853 stay unmarked per explicit instruction not to add markers beyond what was asked.
…registries (#775) - classify every one of the 89 `_missing_` bodies still calling `extend_enum` against the owner's ruling on #775/#847: a final concrete assigned name mints, a notation for the reader (Unassigned/Reserved/Deprecated/etc.) unmints - convert 82 registries wholly and two more (`EtherType`, `Socket`) partially, 172 branches total, from `extend_enum` to `_unregistered_member`, in both the vendor crawler and the generated const file, following #838's/#858's precedent; proved a 5-file sample (Form-A and Form-B crawlers, both mixed registries) regenerates byte-identically - leave 89-82=7 untouched: `CGAType`'s mint is not an IANA-style range at all, and 6 files sit on classes without `EnumRegistry` yet (`AppType` and friends), tracked separately by #860 pending #859 - extend `tests/const/test_const_enum_no_mint.py` with the ruling-derived registry lists and behavioural/source coverage for all 82+2, correct its stale "~92 still mint" docstring to the measured 89, and fix collateral breakage in three sibling test files and `tests/vendor/test_ipx_socket_ unit.py` that pinned the pre-ruling mint behaviour - fix the last two collateral pins the ruling invalidates, in files the first pass missed and CI caught: `tests/corekit/test_fields_numbers_unassigned_ enum.py` expected `BlockType(0x0bad0bad).name == 'Reserved_0bad0bad'`, now `Reserved` with the value asserted explicitly since the name no longer carries it; and `tests/protocols/misc/test_pcapng_unit.py` read `FilterType.Unassigned_0` by attribute, a member that existed only because of the import-time mint at `pcapkit/protocols/misc/pcapng.py:4593` which this change removes Build: plain `unittest` on tests/const (179), tests/vendor (86), tests/corekit/test_fields_numbers_unassigned_enum.py (11) and tests/protocols/misc/test_pcapng_unit.py (92 + 1 skipped) all green; both newly-fixed files fail with their const file reverted to main.
…registries (#775) - classify every one of the 89 `_missing_` bodies still calling `extend_enum` against the owner's ruling on #775/#847: a final concrete assigned name mints, a notation for the reader (Unassigned/Reserved/Deprecated/etc.) unmints - convert 82 registries wholly and two more (`EtherType`, `Socket`) partially, 172 branches total, from `extend_enum` to `_unregistered_member`, in both the vendor crawler and the generated const file, following #838's/#858's precedent; proved a 5-file sample (Form-A and Form-B crawlers, both mixed registries) regenerates byte-identically - leave 89-82=7 untouched: `CGAType`'s mint is not an IANA-style range at all, and 6 files sit on classes without `EnumRegistry` yet (`AppType` and friends), tracked separately by #860 pending #859 - extend `tests/const/test_const_enum_no_mint.py` with the ruling-derived registry lists and behavioural/source coverage for all 82+2, correct its stale "~92 still mint" docstring to the measured 89, and fix collateral breakage in three sibling test files and `tests/vendor/test_ipx_socket_ unit.py` that pinned the pre-ruling mint behaviour - fix the last two collateral pins the ruling invalidates, in files the first pass missed and CI caught: `tests/corekit/test_fields_numbers_unassigned_ enum.py` expected `BlockType(0x0bad0bad).name == 'Reserved_0bad0bad'`, now `Reserved` with the value asserted explicitly since the name no longer carries it; and `tests/protocols/misc/test_pcapng_unit.py` read `FilterType.Unassigned_0` by attribute, a member that existed only because of the import-time mint at `pcapkit/protocols/misc/pcapng.py:4593` which this change removes Build: plain `unittest` on tests/const (179), tests/vendor (86), tests/corekit/test_fields_numbers_unassigned_enum.py (11) and tests/protocols/misc/test_pcapng_unit.py (92 + 1 skipped) all green; both newly-fixed files fail with their const file reverted to main.
get()'s string-key miss and every generated registry's _missing_ bounded-range branch both called aenum.extend_enum() on an unrecognised value, so e.g. Hardware(40) minted a permanent member on first call, forever. Fixed at the two shared sites in pcapkit/vendor/default.py, plus a new register(value, name) classmethod for the caller-named path that is still meant to mint. All 105 generated pcapkit/const/ registries are hand-edited to match and proved by regenerating the 21 changed ones from live IANA data -- byte-identical. get()'s fix applies to all 105 registries; the _missing_ fix only changes runtime behaviour for the 21 whose vendor crawler leaves Vendor.process() unmodified. Two toolkit call sites and four protocol reader sites now raise MissingKeyError/ProtocolError instead of getting a minted placeholder back. 1026 minting call sites remain out of scope, deferred to a later tier.
…ap_type pcapkit/toolkit/pyshark.py resolved a frame's link type from packet.layers[0].layer_name, a Wireshark display-filter name, with a blanket Enum_LinkType.get(name.upper()) fallback. One filter name serves several encapsulations, so that fallback returned wrong DLTs silently -- null for a DLT_LOOP capture, 101 (RAW) for rawip6. Re-keyed onto frame.encap_type, which is distinct per encapsulation, and the fallback is gone: an unrecognised encapsulation or filter name now raises MissingKeyError. Both lookup tables were measured through editcap/tshark round trips rather than transcribed from Wireshark source, which is not on the build host: ENCAP_TYPE_TO_LINKTYPE grows to 152 entries and FILTER_NAME_TO_LINKTYPE grows from #838's 2 entries to 58.
Adds util/pyshark_encap_map.py, a generator that regenerates ENCAP_TYPE_TO_LINKTYPE (152 entries) and FILTER_NAME_TO_LINKTYPE (58) in place inside pcapkit/toolkit/pyshark.py, so the two tables #850 hand-built stop being hand-maintained. Guards against its own worst failure mode: a naive LinkType(dlt) lookup cannot detect an unmapped DLT, because _missing_ mints a placeholder rather than raising, so the generator snapshots every known value before any lookup. The real-tshark sweep is gated on a new HAS_WIRESHARK flag and is version-pinned -- editcap -T accepts 226 encapsulations on Wireshark 4.6.9, 224 on 4.2.2 (CI's Ubuntu noble) -- so the count assertions run only under the measured version. One line of pyshark.py itself changes as a result: the IPMB_LINUX table entry becomes I2C_LINUX, the canonical name #848 gave value 209. util/changelog_md.py regenerated CHANGELOG.md for all seven entries added across this and the six preceding commits (#838, #846, #847, #848, #849, #850, #853); `--check` exit 0.
#846 and #850 - #838's "the 21 whose vendor crawler leaves Vendor.process() unmodified" measures 22, not 21 -- pcapkit.const.hip.transport.Transport also has an unmodified process(), but its FLAG-bound range has no unassigned gap, so its _missing_ carries no bounded-range extend_enum branch to fix. The real discriminator is that branch, not the process() override; reworded, and the "other 84" split into the 83 that do override process() and the one that doesn't but has nothing to fix either. Re-derived independently against 5e25db2^..5e25db2 (not main, which would fold in #847's own edit to ipx/socket.py): 22 registries have no process() override, 21 of them have the bounded-range branch, matching tests/const/test_const_enum_no_mint.py's own REGISTRIES_WITH_UNASSIGNED_RANGES (21 entries) vs ALL_REGISTRIES (22, +hip.transport, with a NOTE explaining the exclusion). - #846's "Only tests/integration/test_engine_runtime.py and test_engine_parity.py change" is false -- 30bca99's own numstat also touches .github/workflows/unit-tests.yml (17+/2-). Qualified to "only these two test files change". - #850 gets the breaking marker: its own prose already says the fallback is gone and an unrecognised encapsulation or filter name now raises MissingKeyError instead of substituting a plausible DLT -- the same shape as the two existing markers at AppType.get (1.5.0.rst:2456) and the four .get()-backed enum fields (1.5.0.rst:2720). Restructured to lead with the marker and the subject, matching their wording and placement; the later restatement of the same fact is dropped. None of these are code or test changes -- prose only, matching the cross-review's own framing. #848 stays unmarked (its own "Not breaking" paragraph holds); #838, #849 and #853 stay unmarked per explicit instruction not to add markers beyond what was asked.
#719 Swept tests/ for owner-ruling quotes attributed to the wrong GitHub thread, the same defect class #719 fixed under pcapkit/. Confirmed each by grepping the quote's distinctive text against the cited thread's body/comments; a hit elsewhere is a re-quote or a different thread's own words, not the source. - test_sentinel_exports_unit.py: the already-reported #937->#719 fix for AbsentType's privacy ruling. - test_enum_lookup_reparent_930_unit.py (4 sites) and test_mh_unit.py: "I prefer (2) directly" and the question that drew it are in pull request #940's thread, not issue #935 -- #935 only carries the first ruling ("I lean on 1"). - test_vendor_snapshot_restore_unit.py: the contextlib/atomic-write ruling is in pull request #873's thread; issue #872 has zero comments. - test_vendor_reg_apptype_generator_unit.py (2 sites): the "undefined direct uses 0" ruling is in pull request #874's thread, not issue #860 or #770. - test_const_enum_no_mint.py (2 sites): the mint/unmint criterion was settled on pull request #847 and confirmed on #775 -- the reverse of what the text said, per #861's own description of the same ruling; and "Q1 - bare it is." is pull request #838's thread, not #775's. One occurrence left unresolved rather than guessed at: the "Preserve each branch's existing name argument..." quote (4 sites in test_const_enum_no_mint.py, attributed to "#775's final round") does not appear verbatim in #775, #847, or #878 (the implementing PR) by body, comments, review comments, or commit message -- only a paraphrase in #878's own PR description/commit message, which is the author's prose rather than a quoted ruling. Flagged for the owner rather than fixed. tests/corekit/, tests/vendor/, tests/const/ pass (400/16, 118, 299 respectively, pcapkit.__file__ confirmed inside this worktree); tests/protocols/internet/test_mh_unit.py passes standalone (52/0) -- the full directory has 5 unrelated pre-existing failures from ungenerated examples/captures/ fixtures, untouched by this change. Refs #719
…quest (#719) (#982) * docs(pcapkit,ci): cite the issue a defect belongs to, not the pull request (#719) Per the owner's ruling on #719, replace every reference to a pull-request number in pcapkit/** and .github/workflows/** comments and docstrings with the issue it closed, or a description where no issue covers it. - 92 real PR citations in pcapkit/ (93 was the estimate; the gap is RFC packet-diagram and hex-format-spec false positives, plus one cross-repo issue citation that only coincidentally matched a PyPCAPKit PR number). - 13 PR citations in .github/workflows/, matching the estimate exactly. - Several citations named two or three numbers for one claim where a PR closed several issues, or several PRs closed the same issue; deduplicated rather than left reading "#425 and #425". Four review rounds caught the same category error recurring: several sites had relocated a verbatim quote or a specific finding into the issue number rather than describing where the ruling was actually given, so the quote no longer existed where the sentence pointed. Fixed each by naming the issue while locating the ruling honestly -- "a ruling given in review of the work for #N" -- the same shape already used on this repo's conventions docs. Two sites needed the inverse correction instead: the #923 quote in enum.py/exceptions.py genuinely is recorded on #923's own thread, just attributed there to the pull request that implemented it, so those read "a ruling recorded on GitHub issue #923" rather than pointing elsewhere. Also fixed a lost conjunction and an ordinal/number mismatch in corekit/enum.py, a self-contradicting below/above pointer repeated across three internet/ files, and a number collision in http.py where one issue ended up naming both a defect and the change that closed it. Final sweep: grepped the whole tree for the word "verbatim" -- the marker that makes a quote-attribution claim falsifiable -- across all 30 files under pcapkit/ that carry it, and checked every quote this way names against the actual issue thread. Caught two more of the same defect: vendor/__main__.py's #872 citation (the quote is in the implementing pull request's review, not #872 itself) and four sites across mh.py attributing to #935 a ruling that only exists in the review of the pull request that implemented it -- #935's own thread holds just the superseded widen-not-delete proposal. Both fixed the same way. Every other quote-bearing claim the sweep found -- #911, #937, three distinct #877 quotes, both #842 quotes, and the #860/#808/#806/#886/#917 rewrites from earlier in this pass -- resolves to the thread it names. Verified: targeted pytest across every touched module passes, including the test that pins the vendor/const apptype.py get() region as byte-identical, reconfirmed after each amendment. Both edited workflow YAML files parse before and after with unchanged key counts. * docs(corekit): cite #719, not #937, for the AbsentType ruling AbsentType's docstring attributed the owner's "document it as private type/class... not for public use is enough" quote to #937. #937 itself quotes that ruling verbatim under "The owner's ruling, verbatim (from #719)" -- it re-attributes rather than originates it. Per the house rule to cite the issue a ruling was settled on (docs/source/contributing/conventions/documentation.rst:196-200), point the attribution at #719 and re-wrap the paragraph to the file's existing ~78-column width. The neighbouring, unrelated #937 citation describing what #937 did to sentinel naming is untouched. tests/corekit/ passes (400 passed, 16 skipped) against this worktree's own pcapkit (confirmed via pcapkit.__file__); pylint on the file is 9.77/10, unchanged by this edit -- the one finding is a pre-existing, unrelated too-few-public-methods warning on NoValueType. * docs(tests): re-point six ruling citations at their actual threads, per #719 Swept tests/ for owner-ruling quotes attributed to the wrong GitHub thread, the same defect class #719 fixed under pcapkit/. Confirmed each by grepping the quote's distinctive text against the cited thread's body/comments; a hit elsewhere is a re-quote or a different thread's own words, not the source. - test_sentinel_exports_unit.py: the already-reported #937->#719 fix for AbsentType's privacy ruling. - test_enum_lookup_reparent_930_unit.py (4 sites) and test_mh_unit.py: "I prefer (2) directly" and the question that drew it are in pull request #940's thread, not issue #935 -- #935 only carries the first ruling ("I lean on 1"). - test_vendor_snapshot_restore_unit.py: the contextlib/atomic-write ruling is in pull request #873's thread; issue #872 has zero comments. - test_vendor_reg_apptype_generator_unit.py (2 sites): the "undefined direct uses 0" ruling is in pull request #874's thread, not issue #860 or #770. - test_const_enum_no_mint.py (2 sites): the mint/unmint criterion was settled on pull request #847 and confirmed on #775 -- the reverse of what the text said, per #861's own description of the same ruling; and "Q1 - bare it is." is pull request #838's thread, not #775's. One occurrence left unresolved rather than guessed at: the "Preserve each branch's existing name argument..." quote (4 sites in test_const_enum_no_mint.py, attributed to "#775's final round") does not appear verbatim in #775, #847, or #878 (the implementing PR) by body, comments, review comments, or commit message -- only a paraphrase in #878's own PR description/commit message, which is the author's prose rather than a quoted ruling. Flagged for the owner rather than fixed. tests/corekit/, tests/vendor/, tests/const/ pass (400/16, 118, 299 respectively, pcapkit.__file__ confirmed inside this worktree); tests/protocols/internet/test_mh_unit.py passes standalone (52/0) -- the full directory has 5 unrelated pre-existing failures from ungenerated examples/captures/ fixtures, untouched by this change. Refs #719 * docs(tests): paraphrase four fabricated or altered owner quotations (#719) Per #719's citation ruling (de-quote, never reproduce a verbatim quote that may have come from outside GitHub): - test_const_enum_no_mint.py (4 sites): a quotation attributed to "the owner's ruling, verbatim" never appears in #775, #847, #861 or #878 (or anywhere in the repo's comment corpus). Replaced with a paraphrase attributed to PR #878's own body, which carries the real design note in different words. - test_sentinel_exports_unit.py / test_const_registry_protocol.py: a quote attributed to #911 silently dropped half of what the owner wrote on #719 and swapped `__all__` for "users". Replaced with a paraphrase naming #719 as where it was settled and #911 as the issue that carried it out. - test_const_enum_no_mint.py / test_const_enum_builtin_parity.py (4 sites): a "verbatim" quote of #860 silently corrected the owner's typo ("entires" -> "entries"). Paraphrased, which drops the question of reproducing or flagging the typo. - test_enum_lookup_reparent_930_unit.py: "the owner's final ruling there" had #935 as its nearest antecedent instead of #940; named #940 explicitly and paraphrased the adjacent quote. Verified: ast.parse and reST markup pairing clean on every touched file; tests/const (299 tests) and the targeted pytest sweep of all touched files (261 passed, 2013 subtests) are green. tests/corekit's full discover run shows 5 pre-existing failures in test_sentinel_exports_unit.py, confirmed identical on the unedited originals -- a cross-file test-order dependency unrelated to this change. * test(vendor,corekit): fix a surviving fabricated ruling and a wrong citation (#719) - tests/vendor/test_ipx_socket_unit.py: the "owner's ruling" attribution for keeping the hex-suffixed Xerox name survived in this file after the prior commit removed the same false attribution from four sites in test_const_enum_no_mint.py. Reworded to credit PR #878's own design note, matching the wording already used at the repaired sites. - tests/corekit/test_sentinel_exports_unit.py: the docstring cited the #719 export ruling ("only export objects, not types") as grounds for keeping ABSENT out of __all__, but ABSENT is an object, so that ruling argues for including it, not excluding it. Re-grounded the sentence on the privacy ruling already quoted ~15 lines below instead, without re-quoting it. Both changes are prose-only: tokenizing each file before and after with comments and docstrings stripped produces identical token sequences. tests/vendor passes 118/118 except one pre-existing, test-order-dependent flake in test_vendor_snapshot_restore_unit.py (reproduces identically on the pre-edit tree); tests/project/test_conventions_doc_claims.py passes 38/38. * test(corekit,const): narrow the blanket paraphrase, restoring quotations that cite correctly (#719) The last two commits paraphrased every disputed owner quotation away. That was right for one case and wrong for two: a quotation that exists nowhere has to be paraphrased, but a quotation that is real and was only cited to the wrong thread lost its audit trail for nothing, since the defect was the pointer, not the words. Per the owner's ruling, narrow the fix to match. Restored as quotations, correctly cited: - tests/corekit/test_sentinel_exports_unit.py (~L4-7) and tests/const/test_const_registry_protocol.py (~L1363): the sentinel export rule, split back into its two real sources instead of one spliced sentence -- #719's "we should ONLY export the objects ... and leave the types ... out", and #911's own "we only expose the final objects to users", with #911 noted as both executor and source. - tests/const/test_const_enum_no_mint.py (~L88, ~L1876, ~L2363) and tests/const/test_const_enum_builtin_parity.py (~L655): the #860 minting ruling, including its load-bearing first sentence ("I think we should not mint on get still actually") and the owner's own "entires" typo, marked [sic] rather than silently corrected. Left alone: the four #878 fabricated-quote sites in test_const_enum_no_mint.py, which cite no real thread and stay paraphrased, and the ABSENT privacy sentence, which is a correct paraphrase of a different ruling. Verified: ast.parse and reST markup clean on all four files; code token sequences (docstrings/comments stripped) identical before and after; each restored quotation substring-matches its source comment after whitespace/markup normalisation. tests/const: 299 OK. tests/ project/test_conventions_doc_claims: 38 OK, 1 skipped. * test(corekit,const): convert restored quotations to statements with context, per #719 The previous commit restored eight verbatim quotations to fix a narrowing that had dropped their context. The owner has since ruled that neither form is right: a narrowed paraphrase without context does not help a reader who was not in the thread, but a verbatim quotation makes the docstring read as a discussion rather than documentation. - Sentinel export rule (corekit/test_sentinel_exports_unit.py, const/test_const_registry_protocol.py): state that a module's `__all__` lists a sentinel's object but deliberately leaves its type out, and why (the type is not part of the public surface), citing #719 as where it was settled and #911 as where the implementing work belongs. - #860 minting rule, four sites (const/test_const_enum_no_mint.py x3, const/test_const_enum_builtin_parity.py): state that `get()` must not mint and only `register()` creates a new entry, and why (only IANA-registered values are legitimate and `get()` lacks the information to construct one), citing #860. Each site is fitted to its own surrounding prose rather than one paragraph pasted four times. Drops the `[sic]` each quotation carried, since there is nothing left to reproduce. - Fixed two sentences left orphaned by the quotations' removal: an antecedent ("the three") that depended on the deleted quote's wording, and a sentence whose "get() as well as _missing_" had the emphasis backwards relative to the rule's own subject. The four PR #878 paraphrase sites in test_const_enum_no_mint.py were already in this third form and are unchanged. Verified: ast.parse on all four files; tokenize with comments and docstrings stripped shows an identical token sequence before/after (prose-only); tests/const (299) and tests/project/test_conventions_doc_claims.py (38, 1 skip) pass; tests/corekit (400, 5 failures, 16 skipped) matches the documented pre-existing sentinel-identity failures. * test(corekit,const): state the remaining owner rulings in our own words, per #719 The four files still carried owner-attributed quotations beside the eight converted earlier, so each read half as documentation and half as a thread. - Replace each quoted ruling with a statement of the rule, the reason a reader needs, and the issue where it was given (#842, #864, #775, #860, #911, #719, #647, #808, #759, #857). - Rename the dangling "privacy ruling quoted below" reference to point at the statement that replaced the quotation. - Leave RFC text, code literals and ordinary prose untouched. Prose only: tokens with comments and docstrings stripped are identical before and after; tests/const 299 OK, tests/corekit unchanged (5 known). * test(const): restore ruling citations to the pull requests that carry them, per #719 tests/ is exempt from the issue-citation rule (documentation.rst, ruled on #719): the fact cited lives in the pull request, not the issue. - PR #836 restored for the TransportProtocol-extension refusal, the |-composite decoding retirement, and the stale-comment deletion; the rulings are not on #808 at all. - PR #783 for the f-string convention; PR #847 for the mint criterion. - The de-quotation stands: wording stays as statements, no quotation marks.
1 participant
Please follow the guide below
make testpasses, and a test case covers the changeWhat is the purpose of your pull request?
fix— corrects a defectDescription of your pull request and other information
Tier 1 of #775. The maintainer's ruling: an unrecognised value/key should
never permanently register a new member unless the caller explicitly asks
for one. Today both
get()'s string-key miss and_missing_'s boundedrange branch call
extend_enum(), so e.g.Hardware(40)mints apermanent member on first call.
This edits the two shared sites in
pcapkit/vendor/default.pyand adds abase
register(value, name)classmethod, the explicit caller-named paththat still mints — value-first, matching
_unregistered_member(value, name)'s own order, so the two no longer disagree. 105 generatedpcapkit/const/modules are hand-edited to match, then proved equivalent byregenerating all 21 changed registries from live IANA — the resulting diff
is byte-identical to the hand-edit. Seven further registries across both
template kinds regenerate with no change at all, so no upstream drift is
riding along. (An earlier revision of this description claimed regeneration
was not possible offline; that was wrong — the network is reachable and
Vendor.__init__crawls and rewrites on construction.) New tests intests/const/test_const_enum_no_mint.pycover theno-mint behaviour and
register()/_unregistered_member()across all 105registries. Pickling an unregistered member works by re-derivation rather
than identity, so two equal unregistered members are not the same object.
Public behaviour change:
get()'s no-longer-minting miss path appliesto all 105 registries this PR edits.
_missing_'s bounded-range fix,however, only actually changes behaviour for the 21 registries whose
vendor crawler leaves
Vendor.process()unmodified (REGISTRIES_WITH_UNASSIGNED_RANGESinthe new test module) — those are the ones where an unregistered value now
stays absent from
__members__/_value2member_map_. The other 84 editedfiles'
_missing_is unchanged from base: 83 of them overrideprocess()with their own bespoke logic and still call
extend_enumthere, and thelast one shares the same unmodified template but has no unassigned range
to mint against in the first place.
This PR also fixes two toolkit call sites that fed
get()a string keywith no default and so now raise where they used to get back a minted
placeholder —
pcapkit/toolkit/scapy.pyandpcapkit/toolkit/pyshark.pynow letthat
KeyErrorthrough asMissingKeyErrorinstead, sinceLinkType.NULLand
LinkType.RAWare genuine DLTs with their own handler protocol classesand neither is an honest stand-in for "unknown link type" — and four protocol reader
sites (
hopopt.py/ipv6_opts.py's SeedID and TaggerID lookups) that nowconvert the same
KeyErrorinto their ownProtocolError. That guard is foran unresolvable string key from a hand-built schema, not a wire value — an
unassigned wire value still mints through
_missing_and is deferred to tier 2.(An earlier revision of this description said "wire value", which overstated
the scope and contradicted the commit message.)
Out of scope here, now counted exactly by an AST census rather than
estimated: 1026 minting call sites remain — 1007 in
_missing_and 7in
getacrosspcapkit/const/**, plus the 12 hand-written ones below.register/register_aliasaccount for a further 106 sites which are theby-design caller-named mint path and are deliberately untouched.
Two things that census corrected.
pcapkit/vendor/**holds zero realcall sites —
grepfinds 105 hits in 91 files, but every one is insidean f-string template the crawler writes into a generated file, so any
grep-based figure for this work is wrong by construction. And editing
the shared template again would fix nothing further: of the 26 vendor
modules that use
pcapkit/vendor/default.pyuntouched, none generates astill-minting const file, while all 89 that do come from the 95 modules
carrying their own
process()or template.AppTypealone is 767 of the1026 sites.
One more population, added after review: 12
extend_enumsites liveoutside
pcapkit/const/andpcapkit/vendor/entirely, in twohand-written protocol modules —
pcapkit/protocols/internet/mh.py(
:618,:632,:676,:690,:731,:745,:782,:796) andpcapkit/protocols/application/ngap.py(:366,:379,:847,:860).These carry the exact pre-#838
get/_missing_pattern, and no vendorregeneration will ever reach them. All left for tier 2.
Per the maintainer's ruling, the pyshark name mismatch is fixed here rather than
deferred: pyshark reports Wireshark's PDML filter name, not a DLT name, so
FILTER_NAME_TO_LINKTYPEinpcapkit/toolkit/pyshark.pytranslates the two namesthat are unambiguous against Wireshark's own dissector registrations —
eth→ETHERNETandtr→IEEE802_5, each bound to exactly oneWTAP_ENCAP_*. It isconsulted before
LinkType.get, which still resolves a filter name that alreadyspells a member (
ppp,fddi— verified).sll,fr,ipandipv6aredeliberately absent, though for different reasons — measured, not assumed:
sllis genuinely shared, one filter name for both thelinux-sll(DLT 113) andlinux-sll2(DLT 276) encapsulations.ipandipv6are absent because they neverarrive as the root layer at all. And
frturns out not to be ambiguous — both itsencapsulations write DLT 107 — so it is mappable and is left out only as unneeded scope. A genuinely unknown name still raises.
Corrections found across review rounds, all measured with tshark 4.6.9 and
editcap -T:rawdoes not raise — it servesRAW/IPV4/IPV6(101/228/229) and the fallback answers101 for all three.
nulldoes not raise — servesNULL/LOOP(0/108), answers 0. Andip/ipv6never arrive as the root layer at all: across the 158 of the 226 encapsulationseditcap -Taccepts that an Ethernet source can be rewritten into, neither is ever
layers[0], because a raw IPv6 capture roots atraw. Those silentwrong answers are #843, not something this PR introduces.
Refs #840 rather than closes it. That issue also asks for
ip→IPV4andloop→NULL. Onlyipis refused.loopalready resolves toLinkType.LOOP(108)through the fallback — and is the wrong filter name anyway: tshark registers
loopasConfiguration Test Protocol (loopback), an Ethernet payload, while a
DLT_NULLorDLT_LOOPcapture presents
null.nullresolves toLinkType.NULL(0), so aDLT_LOOPcapture silentlyreads as DLT 0. That ambiguity is #843. #840 stays open.
Generalising
register_aliassoETHcould instead be a real alias onLinkTypeis #842, filed separately at the maintainer's direction.