fix(const,vendor): Old Xerox ethertype range shadowed by IEEE802.3 range

[JarryShaw]

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort) — not run locally in this session; diff follows the style of the surrounding, unchanged code
• make test passes, and a test case covers the change — ran via coverage run -m pytest on tests/const (188 passed, 40027 subtests) and tests/vendor (86 passed, 189 subtests); did not run the full suite (documented OOM risk on this tree)
• Added a changelog entry — 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

Fixes #862. EtherType._missing_ tested 0x0000 <= value <= 0x05DC (IEEE802.3
Length Field) before 0x0101 <= value <= 0x01FF (Old Xerox Experimental). The
second range is wholly contained in the first, and the generated method
returns on the first matching if, so the Old Xerox branch was unreachable
for every value it covers — EtherType(0x0101).name resolved to
'IEEE802_3_Length_Field_0x0101' instead of the Old Xerox name.

The range bounds come from the live IANA CSV rather than a literal table
(unlike #841's IPX socket fix), so the fix in
pcapkit/vendor/reg/ethertype.py is general: process() now collects each
range row and a new EtherType._insert_range places each range ahead of the
first already-placed range that fully contains it, leaving every
non-overlapping pair in the CSV's own row order. pcapkit/const/reg/ethertype.py
is regenerated (python -m pcapkit.vendor reg.ethertype); the diff is exactly
the two branches swapping places.

Swept all 56 range tests in the generated _missing_: this pair is the only
containment among them, so no other subsumed range exists today.

New tests in tests/const/test_const_ethertype_862_unit.py pin both the
symptom (against the committed const file) and the root cause (against
process() fed a CSV fixture reproducing IANA's own rows), plus the general
ordering rule against synthetic nested ranges. Verified each fails against
the pre-fix generator/const file.

[JarryShaw]

The 6 red checks here are not this PR's defect — they are #866's, inherited from main. Recording the evidence so nobody re-diagnoses it.

CI builds the PR merge ref, not the head. Run 36356122072 reports head_sha=d1626d348, but refs/pull/865/merge is a94b9687a with parents 946b84e83 (current main) and d1626d348 (this branch). That merged tree carries main's broken _missing_ body:

# a94b9687a:pcapkit/const/pcapng/secrets_type.py
        cls._unregistered_member(value, 'Unassigned')
        return cls(value)

whereas this branch's own tree has the correct return cls._unregistered_member(...) first, because it branched from 70fa92010 — before e58618bdf regenerated the two pcapng registries from their stale crawlers.

Every failure is that recursion, not anything in the ethertype change:

SUBFAILED tests/const/test_const_enum_lookup.py::ConstEnumZeroLookupTests::test_zero_lookup_matches_the_registry
  AssertionError: SecretsType(0) unexpectedly raised RecursionError(...)
SUBFAILED tests/const/test_const_enum_no_mint.py::RulingConversionDoesNotMintTests::test_converted_value_does_not_mint
  RecursionError: maximum recursion depth exceeded

Five Python 3.x legs plus Required checks passed, which fails because they did — one defect, six marks, none of them ours.

#865 is therefore gated on #867 merging, after which this needs an update to pick up the fix before its CI can be read. Keeping review: pending — the cross-review in flight is against the branch's own tree, which is unaffected.

Worth stating as a general trap, since it cuts both ways: because CI tests the merge ref, a PR can go red for a defect it does not contain, and can go green while carrying one that only appears once main moves. The sha in head_sha is not the sha that was tested.

[JarryShaw]

NEEDS CHANGES on d1626d348 — cross-review (opus; author sonnet). The fix itself is right and all seven claims verified; what is missing is collateral in an adjacent file the PR does not touch.

tests/const/test_const_enum_no_mint.py documents, in three places, the masking this PR removes — verified verbatim by me. Nothing fails, because the surviving assertion is an order-insensitive source-text check, so CI cannot catch it:

• :355-361 — EtherType's exclusion from RULING_CONVERTED_WITH_REACHABLE_GAP justified by its first converted branch being "unreachable, masked by the wider 0x0000-0x05DC branch before it".
• :444-450 — "not behaviourally probeable … left as a follow-up rather than reordered here". This PR is that follow-up.
• :952-955 — test_masked_old_xerox_row_converts_by_source's docstring: "unreachable at runtime … Proved by source instead of by calling it."

The third is a testing gap, not just stale prose: 0x0101 is now behaviourally probeable, so it can join ETHERTYPE_UNASSIGNED_PROBES (today {0x8039: 'DEC_Unassigned'}) and get the strictly stronger no-mint proof test_unassigned_rows_do_not_mint already gives DEC — instead of a source assertion that would still pass if the ordering regressed.

Everything else confirmed independently, and two claims came back stronger than the PR made them. _insert_range is general to arbitrary depth, not just two levels: all 6 arrival orders at 3 levels, 0 of 24 failing at 4 levels, 0 of 4000 randomized fuzz cases, with a transitivity proof for why inserting before the first container cannot strand a later element. And the file is exhaustively safe — 56 ranges, 1 containment pair, 0 partial overlaps, 0 identical pairs — so since first-match-wins can only matter where ranges intersect, nothing else in the chain depends on ordering.

Two latent warts, neither blocking and neither present today: identical ranges silently reverse precedence (rlo <= lo and hi <= rhi is non-strict, so an identical range counts as containing; pre-fix the earlier CSV row won, post-fix the later one does), and partial overlap is not handled by design — correct, since neither range is wholly dead, but nothing detects one if IANA adds it.

One nit in a file the PR owns: test_nested_ranges_are_ordered_narrowest_first feeds OUTER→MIDDLE→INNER, where every insert lands at index 0, so a naive "insert at front" rule passes it too. Arrival order INNER, OUTER, MIDDLE is the only shape needing a mid-list insert.

breaking confirmed, on firmer grounds than I had: besides .name changing for all 255 values, enum identity flips — EtherType(0x0101) is EtherType(0x0101) was True (minted and cached) and is now False, _value2member_map_ membership goes True→False, and lookups stop growing the registry (161→160). Not a dissection break (Ethernet dispatches on packet['type'] and nothing is registered for that range either way), but a real contract change for any caller reading .name or relying on identity.

Also now verified rather than UNVERIFIED: pylint 10.00/10 on both changed source files, mypy clean on the vendor one, isort clean on all three — changed files only, package-wide still unverified. #866's defect class is not present here: 57 return statements for 56 range branches, so no branch can fall through, and _unregistered_member never calls cls(value). And the PR merges cleanly onto current main — its three files are byte-identical at 70fa92010 and 946b84e83, and its 9 tests pass when laid onto a 946b84e83 tree, so no interaction with #863.

[JarryShaw]

Round 2 pushed — head 8901e28f1, same single commit, now 4 files: the two ethertype sources unchanged at 3/3 and 35/5, plus tests/const/test_const_enum_no_mint.py 61/39 and the new test file grown to 232 lines.

All three stale sites addressed, and the author went further than asked on the first one rather than just deleting a sentence. It read the other reason EtherType was excluded from RULING_CONVERTED_WITH_REACHABLE_GAP ("13 with no reachable gap at all" — inapplicable, EtherType has plenty), then checked whether being a mixed registry independently disqualified it: it does not, since ('pcapkit.const.ipx.socket', 'Socket') is already a member and is also mixed. So EtherType now joins the set, and test_no_reachable_gap_registries_no_longer_call_extend_enum's len(excluded) == 13 still holds — now as the true 13 rather than with EtherType riding along on a coincidental _MIXED_REGISTRIES subtraction.

The probe table gained the behavioural coverage, which was the valuable half:

ETHERTYPE_OLD_XEROX_LABEL = 'Old_Xerox_Experimental_values_Invalid_as_an_Ethertype_since_1983'
ETHERTYPE_UNASSIGNED_PROBES = {
    0x8039: 'DEC_Unassigned',
    0x0101: ETHERTYPE_OLD_XEROX_LABEL,
}

so test_unassigned_rows_do_not_mint's existing subTest loop now covers 0x0101 behaviourally. test_masked_old_xerox_row_converts_by_source was retired as subsumed rather than renamed — the behavioural probe catches both symptoms a regression would produce (minting and the wrong name) where the order-insensitive source check would keep passing — and replaced with an in-place comment saying what it proved and where the coverage went, not silently deleted. The constant was renamed off ETHERTYPE_MASKED_UNASSIGNED_LABEL since "MASKED" is no longer true.

The non-discriminating test is fixed: arrival order is now INNER, OUTER, MIDDLE, the only one of the three forcing a genuine mid-list insert (MIDDLE lands at index 1).

Counts: the new file (9) plus test_const_enum_no_mint.py (20, down from 21 for the retirement) = 29 passed, 0 failures; tests/const 187, tests/vendor 86, no regressions.

Re-review dispatched on the same head. The six red CI checks remain #866's inherited defect, not this PR's — #867 clears them.

One pre-existing inconsistency surfaced and deliberately not touched: RULING_CONVERTED_REGISTRIES's docstring claims "82 registries … including the two mixed registries (EtherType and Socket)", but Socket is absent from that tuple — 81 items. Unrelated to #862; I will file it separately.

[JarryShaw]

NEEDS CHANGES on 8901e28f1 — one comment and one test fixture. Sites 1, 2 and 4 are all correct and site 1 is right for the right reason.

First, a correction to my own previous comment. I wrote that "arrival order INNER, OUTER, MIDDLE is the only shape requiring a mid-list insert". That is false, and I relayed it without deriving it. Worse, "forces a mid-list insert" is not even the criterion that matters. Re-derived myself against the real _insert_range, implementing both naive rules:

order   | real | naiveA(front) | naiveB(last-container) | catches A? | catches B?
O M I   | IMO  | IMO           | MIO                   | no         | YES
O I M   | IMO  | MIO           | IMO                   | YES        | no
M O I   | IMO  | IMO           | MIO                   | no         | YES
M I O   | IMO  | IMO           | IMO                   | no         | no
I O M   | IMO  | MIO           | IMO                   | YES        | no
I M O   | IMO  | IMO           | IMO                   | no         | no

The swap traded blind spots rather than removing one. The old OUTER, MIDDLE, INNER was precisely the order that caught "insert before the last container" — because INNER arrives with two containers already placed, so first- and last-container diverge. The new INNER, OUTER, MIDDLE catches "insert at the front" and loses the other. Net better than round 1, since front-insertion was what I flagged, but the test is not the discriminator its new comment claims.

Required: correct the comment's two false claims, and exercise both arrival orders — I O M excludes the front rule, O M I excludes the last-container rule, and neither alone does both. A four-range chain would cover it in one sequence if you prefer.

The implementation is unaffected: round 1's exhaustive permutation and 4000-trial fuzz already proved _insert_range correct for arbitrary nesting. This is a test-strength defect only.

Site 1 confirmed with the historical premise checked, which is the part I most wanted verified. len(RULING_CONVERTED_REGISTRIES)=81, len(RULING_CONVERTED_WITH_REACHABLE_GAP)=69, RCR - RG is already 13 without the _MIXED_REGISTRIES subtraction, so the assertion now passes as the true 13. And EtherType genuinely belongs: _first_unregistered_value picks 0x101, all four no-mint assertions hold (160/160 members, absent from _value2member_map_). On the pre-fix tree the same probe picked the same value and minted (160→161), so the sweep would have failed — the old exclusion was real, not merely cautious, and is removable only because of this fix.

Site 2's retirement confirmed rather than retracted: the behavioural probe dominates both of the retired source assertions, and adds order-sensitivity the string search never had — the pre-fix run is the proof. Nothing about EtherType was lost in the 39 deleted lines; no dangling reference to the old MASKED constant survives. Counts match: 9 + 20 = 29, tests/const 187, tests/vendor 86, methods not subTest records.

The pre-existing RULING_CONVERTED_REGISTRIES discrepancy is confirmed: 81 entries, Socket absent, while 69 + 13 = 82 shows 82 is intended and the tuple is short. Harmless today. Filing separately, still not part of this PR.

[JarryShaw]

Round 3 pushed — head 7f7b24ef9, same single commit, four files (3/3 + 35/5 ethertype, 61/39 on test_const_enum_no_mint.py, new test 263 lines). The ethertype source is byte-identical across all three rounds — git diff d1626d348 7f7b24ef9 -- pcapkit/ is empty — so every round has been test and prose only.

The author independently re-derived the truth table before changing anything, implementing both naive rules against the real _insert_range across all six permutations, and got the same result: {O I M, I O M} catch naiveA, {O M I, M O I} catch naiveB, {M I O, I M O} catch neither — no single order catches both.

It then took the two-subTest form rather than gambling on an unproven four-range chain: INNER, OUTER, MIDDLE (catches "insert at front") and OUTER, MIDDLE, INNER (catches "insert before the last container"), both asserting [INNER, MIDDLE, OUTER]. The docstring now names which naive rule each order catches, explicitly retracts the false "only one order forces a mid-list insert" claim, and says plainly that this is a regression pin against two specific wrong implementations rather than a general correctness proof — the exhaustive proof (3- and 4-level permutations, 4000-trial fuzz) lives in review evidence, not in the repo. That last distinction is the right one to draw and I would not have thought to ask for it.

Counts, methods not subTest records: 29 for the two files together, tests/const 187, tests/vendor 86, all zero failures.

Label back to review: pending; final re-review dispatched.

A measurement trap worth recording, since it nearly produced a false accusation from me. git diff --numstat origin/main..<branch> on a branch based at 70fa92010 listed CITATION.cff, conda/build, pcapkit/__init__.py, both pcapng const files, pcapkit/corekit/enum.py (2/34) and tests/const/test_const_registry_protocol.py (21/168) — files belonging to #867 and #868. None of those are branch edits. They are main's own drift since the base, appearing as removals on the branch side of a two-dot diff. The merge-base diff shows the true four files. Use origin/main...<branch> or an explicit git merge-base, or a clean branch reads as having deleted 168 lines from another PR's file.

[JarryShaw]

GOOD TO GO on 7f7b24ef9 — round-3 cross-review (opus; author sonnet). Label granted; blocked stays until #867 merges, since the red CI is #866's inherited through the merge ref.

The two-subTest form is sufficient and neither subTest is redundant — each fails under exactly the naive rule it claims and passes under the other:

subTest 1: INNER, OUTER, MIDDLE   real->IMO PASS | naiveA->MIO CAUGHT | naiveB->IMO pass
subTest 2: OUTER, MIDDLE, INNER   real->IMO PASS | naiveA->IMO pass   | naiveB->MIO CAUGHT

Dropping either reopens a blind spot, so the pair is minimal as well as sufficient.

On my "is there a third wrong implementation" question — yes, one, and it is the obvious alternative design. widthsort (append, then stable-sort by range width) passes both subTests and the disjoint test. It is caught, but by a sibling method: against the committed CSV fixture it yields ['Xyplex', 'OldXerox', 'IEEE802_3'], moving Xyplex to the front and destroying the CSV row order this PR promises to preserve, which fails test_unrelated_non_overlapping_range_keeps_table_order. Two further variants (adjacent-container-only, overlap-not-contain) are each caught by one of the two subTests. Five constructed wrong implementations, survivors: none across the class. Not a completeness proof, but nothing got through — and worth knowing that test_nested_ranges_are_ordered_narrowest_first read in isolation discriminates less than the class does.

The retraction reads correctly and, if anything, under-claims — pinning the correct output on two arrival orders excludes an infinite family of misordering implementations, not just the two named. The review would not change it: a test docstring that understates its reach errs in the safe direction, and "regression pin, not a general correctness proof" is the honest characterisation. The provenance note is accurate — the exhaustive proof (six three-level orders, 24 four-level, 4000-trial fuzz) was review evidence and genuinely does not live in the repo.

Counts as methods, independently walked as well as read from testsRun: new file 9, test_const_enum_no_mint.py 20 (29 together), tests/const 187, tests/vendor 86, all zero failures.

git diff d1626d348..7f7b24ef9 -- pcapkit/ is empty — I verified this myself. The ethertype source has not moved since the head first reviewed, so round 1's verdict on the generator, _insert_range's generality, the regeneration idempotency, the breaking grounds, the #866 defect-class check and the lint results all carry through. All three rounds were tests and prose only.

widthsort's escape joins the file pile alongside the RULING_CONVERTED_REGISTRIES 81-vs-82 discrepancy and the two _insert_range warts — none of them this PR's to fix.

[JarryShaw]

Unblocked — #867 merged at 00:10:35Z as 3173c4fb0, and it was the only gate on this PR's CI.

Verified on merged main rather than assumed, tree asserted in a clean worktree:

SecretsType(0)      -> <SecretsType.Unassigned: 0>
RecordType(0xFFFF)  -> <RecordType.Unassigned: 65535>

So the RecursionError that produced every red mark here is gone at source. blocked removed. This PR's own CI needs a fresh run against the new base before it can be read — updating the branch is the next step, and its review: good-to-go verdict stands on the code, which has not moved.

[JarryShaw]

review: good-to-go re-confirmed on the updated head 78ee83577. The branch was brought up to date with a merge commit rather than a rebase, so the label would otherwise be pinned to the superseded 7f7b24ef9 — it is not, and here is why.

$ git diff $(git merge-base 7f7b24ef9 origin/main)..7f7b24ef9   # the reviewed patch
$ git diff origin/main..78ee83577                                # the patch now
  -> identical apart from blob hashes

Same four files, same content: 3/3 + 35/5 ethertype, 61/39 test_const_enum_no_mint.py, 263/0 the new test. Nothing was added by the update, so round 3's GOOD TO GO stands without a fourth review.

Two notes for the merge itself:

The branch now carries a merge commit, so it is two commits rather than the house one-per-PR. Harmless if this lands by squash — which is how #861, #863 and #867 landed (… (#775) (#861), … (#866) (#867)) — but if you prefer a rebase instead, say so and I will squash it back to the single commit. I did not rebase-and-force-push on my own initiative, because that would have thrown away the update you just made.

CI is re-running from scratch on the new base: 10 ok / 0 fail / 48 in flight at the time of writing. The previous six red marks were #866's recursion, which #867 fixed at 3173c4fb0 — verified on merged main, SecretsType(0) → <SecretsType.Unassigned: 0> — so this run should come back clean. I will report if it does not rather than assume.