fix(mh): stop the four flag enums recursing on any non-member value (#623)

[JarryShaw]

Fixes #623.

The defect, re-verified on current main

_missing_ in the four Mobility Header flag enumerations ended in return cls(value) — the same constructor that had just failed to find the value — so every in-range value that is not already a member re-entered _missing_ unbounded.

Measured on 6c3d1b0d9 (the base this branch was cut from), CPython 3.14.7, with the editable install's _EditableFinder stripped from sys.meta_path and the worktree asserted at sys.path[0], pcapkit.__file__ printed as …/.claude/worktrees/agent-a70c31eba5d7b14b1/pcapkit/__init__.py:

--- pcapkit.const.mh.binding_ack_flag.BindingACKFlag ---
  has own _missing_ : True
  defined values    : [2, 4, 8, 16, 32, 64, 128]
  BindingACKFlag(0) -> RecursionError
  BindingACKFlag(1) -> RecursionError
  BindingACKFlag(6) -> RecursionError
  BindingACKFlag(255) -> RecursionError
  BindingACKFlag(256) -> ValueError: 256 is not a valid BindingACKFlag
  BindingACKFlag(-1) -> ValueError: -1 is not a valid BindingACKFlag

All four behave the same way. Two points the issue's framing understates:

• It is not only 0. Every in-range non-member recursed — 1, 6, 255. 0 is merely the most obviously reachable one.
• 0x06 is S|D. These are IntFlag types whose members are single bits, so a composite of two defined bits is also not a member, and also recursed. That is an ordinary value for a flag octet to carry, not an exotic one.

Why it happened, and why the fix is the shape it is

Defining _missing_ at all is the cause. aenum's Flag._missing_ is what resolves zero and composite values — it calls _create_pseudo_member_. Overriding _missing_ shadowed it. pcapkit/const/tcp/flags.py defines no _missing_ and has never had the defect (Flags(0) → <Flags: 0>).

So the fix is to keep the range guard and delegate the rest:

         if not ({FLAG}):
             raise ValueError('%r is not a valid %s' % (value, cls.__name__))
-        return cls(value)
+        return super()._missing_(value)

This is not a second idiom for the same problem — it is the repo's existing one. pcapkit/vendor/default.py:110 emits exactly return super()._missing_(value) as the tail of every generated _missing_, and 75 of the 117 modules under pcapkit/const/ already carry it. The four MH flag templates are hand-rolled LINE templates that never adopted it.

The extend_enum alternative was measured and rejected

The other idiom in the tree mints a member for an unassigned integer. Both candidates stop the recursion; they do not behave the same way:

probe	super()._missing_(value)	extend_enum(cls, 'Unassigned_0x%02x' % value, value)
F(0)	<A: 0>, falsy	<B.Unassigned_0x00: 0>
F(0x06)	<A.D|S: 6>	<B.Unassigned_0x06: 6>
F(0x100)	ValueError	ValueError
_member_map_ after	['B','D','S'] — unchanged	['BB','D','S','Unassigned_0x00','Unassigned_0x01','Unassigned_0x06','Unassigned_0x0e','Unassigned_0xff']

extend_enum hides the composite behind an opaque name and pollutes _member_map_ with an entry per bit pattern — up to 65536 of them for BindingUpdateFlag, whose guard runs to 0xFFFF. Wrong for a flags type, so super() it is.

After

  BindingACKFlag(0) -> <BindingACKFlag: 0>
  BindingACKFlag(1) -> <BindingACKFlag: 1>
  BindingACKFlag(6) -> <BindingACKFlag.S|D: 6>
  BindingACKFlag(255) -> <BindingACKFlag.K|R|P|T|B|S|D|1: 255>
  BindingACKFlag(256) -> ValueError: 256 is not a valid BindingACKFlag
  BindingACKFlag(-1) -> ValueError: -1 is not a valid BindingACKFlag

The guard above the changed line is untouched, so out-of-range and non-integer values raise the same ValueError as before.

Fixed in the vendor templates, then regenerated

pcapkit/const/mh/ is generated output, so a const/-only fix would be reverted by the next crawl. The change is in the four pcapkit/vendor/mh/ templates; pcapkit/const/mh/ was then produced by actually running the crawlers against IANA (all four CSV endpoints returned 200), with a guard that recomputed Vendor.__init__'s target path and refused to proceed unless it landed inside this worktree — the editable install points at the main checkout, and pcapkit-vendor / make vendor resolve through it.

Byte-identical by md5, the standard #511 set — hashes before the regeneration, and md5sum -c after:

2ddc85e2e44bb19177306ff6c06a747c  pcapkit/const/mh/binding_ack_flag.py: OK
5b15dcd1357038d9fdfd63df0390077c  pcapkit/const/mh/binding_update_flag.py: OK
b7a9d04503ed86e30c230f82a6808437  pcapkit/const/mh/handover_ack_flag.py: OK
fc0a68f0abb93af2cc4e8f9a2c7f445a  pcapkit/const/mh/handover_initiate_flag.py: OK

git diff --stat -- pcapkit/const/mh/ after regenerating shows 4 files changed, 4 insertions(+), 4 deletions(-) — the one changed line per file, and nothing else.

Tests

Added to tests/const/test_const_enum_lookup.py, which is where _missing_ correctness already lives. That file's own #492 sweep excludes IntFlag by construction (and not issubclass(obj, IntFlag), "a different value-lookup contract") — which is precisely why #492 did not catch this, in the class of enum it declined to walk. The new ConstFlagMissingRecursionTests is the mirror-image sweep, plus named cases for the four registries: F(0), an unassigned single bit (1), a composite of two defined bits, the surviving range guard, and template↔generated agreement character for character.

Without the fix — all four const/ and all four vendor/ files reverted to HEAD, exit code read from a file rather than a pipeline:

PYTEST_EXIT=1
...
SUBFAILED(enum='pcapkit.const.mh.binding_ack_flag.BindingACKFlag', value='0x6') …::test_a_composite_of_defined_bits_decomposes
SUBFAILED(enum='pcapkit.const.mh.binding_update_flag.BindingUpdateFlag', value='0x30') …::test_a_composite_of_defined_bits_decomposes
SUBFAILED(enum='pcapkit.const.mh.handover_ack_flag.HandoverACKFlag', value='0x60') …::test_a_composite_of_defined_bits_decomposes
SUBFAILED(enum='pcapkit.const.mh.handover_initiate_flag.HandoverInitiateFlag', value='0x30') …::test_a_composite_of_defined_bits_decomposes
SUBFAILED(enum='pcapkit.const.mh.binding_ack_flag.BindingACKFlag') …::test_an_unassigned_single_bit_resolves
SUBFAILED(enum='pcapkit.const.mh.binding_update_flag.BindingUpdateFlag') …::test_an_unassigned_single_bit_resolves
SUBFAILED(enum='pcapkit.const.mh.handover_ack_flag.HandoverACKFlag') …::test_an_unassigned_single_bit_resolves
SUBFAILED(enum='pcapkit.const.mh.handover_initiate_flag.HandoverInitiateFlag') …::test_an_unassigned_single_bit_resolves
SUBFAILED(vendor='pcapkit.vendor.mh.binding_ack_flag') …::test_the_vendor_templates_still_emit_the_fix
SUBFAILED(vendor='pcapkit.vendor.mh.binding_update_flag') …::test_the_vendor_templates_still_emit_the_fix
SUBFAILED(vendor='pcapkit.vendor.mh.handover_ack_flag') …::test_the_vendor_templates_still_emit_the_fix
SUBFAILED(vendor='pcapkit.vendor.mh.handover_initiate_flag') …::test_the_vendor_templates_still_emit_the_fix
SUBFAILED(enum='pcapkit.const.mh.binding_ack_flag.BindingACKFlag') …::test_zero_resolves_to_an_empty_flag
SUBFAILED(enum='pcapkit.const.mh.binding_update_flag.BindingUpdateFlag') …::test_zero_resolves_to_an_empty_flag
SUBFAILED(enum='pcapkit.const.mh.handover_ack_flag.HandoverACKFlag') …::test_zero_resolves_to_an_empty_flag
SUBFAILED(enum='pcapkit.const.mh.handover_initiate_flag.HandoverInitiateFlag') …::test_zero_resolves_to_an_empty_flag
16 failed, 13 passed, 1 warning, 118 subtests passed in 5.63s

with, for instance:

E  AssertionError: pcapkit.const.mh.handover_initiate_flag.HandoverInitiateFlag(0) recursed through _missing_; see GitHub issue #623

With the fix:

PYTEST_EXIT=0
13 passed, 1 warning, 134 subtests passed in 3.94s

16 subtests move from failing to passing (118 → 134), four per enum. The tests assert the resolved value rather than catching RecursionError, so an unfixed tree fails on the error itself.

test_the_range_guard_still_rejects passes both with and without the fix — deliberately, since it pins the part of _missing_ that was already correct.

Scoped runs

Never the whole suite. Exit codes read from files.

run	result
tests/const/ + tests/project/ (post-rebase)	116 passed, 718 subtests passed, exit 0
tests/vendor/	55 passed, 54 subtests passed, exit 0
tests/protocols/internet/test_mh_unit.py	50 passed, 468 subtests passed, exit 0
python util/changelog_md.py --check	exit 0

Coverage

coverage run --source=pcapkit.const.mh -m pytest tests/const/, branch coverage on, at HEAD versus this branch:

module	before	after
pcapkit/const/mh/binding_ack_flag.py	87%	94%
pcapkit/const/mh/binding_update_flag.py	89%	94%
pcapkit/const/mh/handover_ack_flag.py	85%	93%
pcapkit/const/mh/handover_initiate_flag.py	86%	93%
TOTAL	87%	93%

The changed line was previously unexecuted in all four (binding_ack_flag.py missing 61, 73 → missing 61), which is another way of saying nothing tested it. The surviving miss in each is the pre-existing get() default-fallback line, untouched here.

Scope deliberately not widened

git grep -n "return cls(value)" -- pcapkit/const/ pcapkit/vendor/ finds six modules on each side, not four:

• the four MH flag modules — defective, fixed here;
• pcapkit/const/pcapng/record_type.py and pcapkit/const/pcapng/secrets_type.py — not defective. Their _missing_ runs extend_enum(cls, 'Unassigned_0x%04x' % value, value) first, so by the time cls(value) runs the member exists and there is no second _missing_ call. Verified, and left alone. Their vendor side emits the same pair as a miss list (pcapkit/vendor/pcapng/record_type.py:68).

No other enum in the tree shares the defect. Nothing else was found and left unfixed.

For the #616 worker (TCP _flags no-op cast)

pcapkit/const/tcp/flags.py does not share this defect. It defines no _missing_ at all (grep -c "_missing_" pcapkit/const/tcp/flags.py → 0), so it uses aenum's own IntFlag._missing_, and a zero-valued flag construction works on unpatched main:

--- pcapkit.const.tcp.flags.Flags ---
  has own _missing_ : False
  Flags(0) -> <Flags: 0>
  Flags(1) -> <Flags: 1>
  Flags(255) -> <Flags.Reserved_4|Reserved_5|Reserved_6|AE|15: 255>

Flags(-1) resolves to all bits set rather than raising, since there is no range guard — worth knowing, but not this issue.

Relation to #596

#596 (545b174bf) touched these same four vendor templates, but only their get() method — the integer path that dropped the caller's default. It left _missing_ alone, and its own commit message says so. tests/const/test_const_enum_get.py:58-65, added by #596, names this recursion explicitly and routes around it, choosing 1 << 70 as its unresolvable probe because an in-range value would have hit it. This PR is the follow-up that fixes what that comment describes, in the same templates, using the tail #596's own reference template already emitted.

AI Usage

Written with Claude Code. The defect was re-verified, both fix candidates measured against aenum 3.1.17 rather than reasoned about, the regeneration run end-to-end and checked by md5, and the failing-then-passing evidence captured with exit codes read from files. A cross-review subagent on a different model reviewed the change; its verdict is posted as a comment below.

[JarryShaw]

Cross-review verdict: GOOD TO GO

Independent adversarial review by a subagent on a different model (Sonnet) — the authoring model was Opus. It was briefed to falsify rather than bless, given the eight load-bearing claims explicitly, and told that disagreements were the valuable output. It ran read-only: 85 tool calls, no repository file amended, git status --porcelain empty at the end (verified).

Measured against pcapkit.__file__ = …/.claude/worktrees/agent-a70c31eba5d7b14b1/pcapkit/__init__.py, aenum 3.1.17. Worth recording that the reviewer independently walked into the editable-install trap — one of its scripts run as a bare file from /tmp resolved pcapkit to the main checkout via _EditableFinder, was caught by its own assertion, and was re-run correctly. The trap is real.

claim	verdict
C1 super()._missing_(value) is correct and safe for an aenum IntFlag	CONFIRMED
C2 the four const/ files are exactly what the four vendor/ templates generate	CONFIRMED (by a stronger method than mine)
C3 no other enum in the repo shares the defect	CONFIRMED
C4 tcp/flags.py does not share the defect; Flags(0) works	CONFIRMED
C5 the tests fail without the fix and pass with it, counts exact	CONFIRMED
C6 the tests are not tautological or over-fitted	CONFIRMED, with one caveat below
C7 coverage did not go backwards	CONFIRMED
C8 scope is 11 files; every numeric claim in the changelog checks out	CONFIRMED

House rules: all satisfied — author, one commit, Fixes #623, no GH-nnn, docs in .rst with CHANGELOG.md as the generated exception.

What it verified that I had not

C1. It read aenum/_enum.py rather than trusting behaviour: Enum.__new__ → _missing_value_ → _missing_, where a None result raises ValueError and anything else must be an instance of cls. IntFlag's _boundary_ is KEEP and none of the four modules override it (grepped, no output), so _create_pseudo_member_ always returns a real cached member on that path and never None. It then checked the properties I had not: the result is hashable and usable as a dict key, correct under & and |, and repeated lookups return the identical object (is True across 512 calls; _member_map_ stayed at 7 named members while _value2member_map_ grew to 256 — aenum's per-value cache, not a per-call leak). And sys.setrecursionlimit(40) still resolves BindingACKFlag(77), so no recursion sensitivity survives.

C2. It judged my proof method weak — correctly — and replaced it with a better one. Rather than re-feeding the committed ENUM block into LINE, it drove the real code path with zero disk writes: cls.__new__(cls), then _request() against the four live IANA CSVs, count(), context(), and the same normalisation Vendor.__init__ applies. All four matched byte-for-byte. That validates the registry content against IANA, which my method could not.

C3. It categorised the last line of _missing_ across all 115 const/ modules that define one: 75 end return super()._missing_(value), 27 end in a returning extend_enum(...), 9 more in a differently-formatted one, and exactly 2 end return cls(value) — pcapng/record_type.py and secrets_type.py. It confirmed those two are sound by construction and by measurement (RecordType(0x1234) → Unassigned_0x1234, same object on repeat, fine at setrecursionlimit(40)). It also swept for spellings my grep would miss and extended the search to pcapkit/protocols/, the only other place _missing_ is defined: ngap.py (3) and mh.py (4) all end in a bare raise or extend_enum. Nothing else in the tree has the pattern.

C4. Confirmed for the #616 worker, and enumerated the whole population: of 117 files under pcapkit/const/, exactly two define no _missing_ at all — tcp/flags.py and ipv6/extension_header.py. All 7 IntFlag classes checked; only the four MH ones had an override.

C6. It ran the experiment I should have run: reverted only the four vendor/ files, leaving const/ fixed. Caught — 4 failed, SUBFAILED(vendor=…) on all four. So the template-agreement test is not vacuous for the regression it targets. It also verified every entry in the hardcoded composite table against the real committed member values.

The one caveat, and why I am leaving it

test_the_vendor_templates_still_emit_the_fix derives the enumeration block from the already-committed module rather than from a crawl. It therefore proves template↔generated boilerplate agreement — which is what it is for, and which it demonstrably catches — but it cannot prove the registry content still matches IANA.

Leaving it as written, deliberately, for two reasons. It matches the existing precedent in tests/const/test_const_enum_get.py::test_the_vendor_template_still_emits_the_fix, added by #596, which does the same thing. And the alternative makes a unit test depend on four live IANA endpoints; the repo's only byte-identity regeneration tests (tests/vendor/test_ipx_socket_unit.py, from #511) exist precisely because those crawlers have LINK = None and need no network, and one of them actively asserts requests.get is never called. The reviewer raised it as optional strengthening rather than a required change, and I agree with that framing — but the limitation is worth stating rather than leaving implied, so it is stated here.

Not attempted

No mutation testing, and no fuzzing of the guard's boundary arithmetic beyond the probes above. Nothing observed suggests either would change the verdict.

Pre-existing, out of scope, not fixed

The raise ValueError(...) in the guard above the changed line is a bare stdlib exception rather than something from pcapkit.utilities.exceptions — which does define an unused EnumError. That is repo-wide: 113 of the 117 const/ modules do it. Fixing it means changing the shared Vendor.__init__ LINE template and regenerating all 117 modules, so it belongs in its own change, not in a narrowly targeted recursion fix. Recorded here so it is not rediscovered at full cost.