Skip to content

fix(const): give the six unguarded registries the same bare-ValueError guard - #677

Merged
JarryShaw merged 1 commit into
mainfrom
fix/const-enum-guard-consistency-647
Sep 23, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/const-enum-guard-consistency-647

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Resolves the inconsistency #647 counted, in the direction the owner set: "keep const enums mock the actual default built-in enum's behaviour, with one exception: they contain the missing then register fallback (mutable enums)".

So the 113 modules that raise a bare ValueError are correct and untouched — that is exactly what enum.IntEnum raises for a value it does not define. The issue's proposal to swap in EnumError is not taken: EnumError is (BaseError, TypeError) at pcapkit/utilities/exceptions.py:276 and is not a ValueError, so it would have diverged from the built-in and sailed past the except ValueError in all 113 generated get() bodies, silently undoing #584.

The real counts, recounted rather than taken from the issue

117 generated modules (134 .py less 17 __init__.py), and 123 registries in them — 111 non-flag IntEnum, 7 IntFlag, 5 StrEnum. The issue's 113/117 is right at module granularity and at the level of the guard's source text. Measured behaviour at registry granularity finds more, because two of the four unguarded modules hold several registries and one guarded module holds an unguarded helper:

registry file:line (pre-fix) Cls(-1) did
Flags pcapkit/const/tcp/flags.py:22 returned 65520Reserved_4|…|FIN, every declared flag at once
CommandType pcapkit/const/ftp/command.py:53 returned 7A|P|S
TransportProtocol pcapkit/const/reg/apptype.py:26 returned 15tcp|udp|sctp|dccp
Command pcapkit/const/ftp/command.py:304 raised AttributeError: 'int' object has no attribute 'upper'
FEATCode pcapkit/const/ftp/command.py:43 raised AttributeError
Method pcapkit/const/http/method.py:182 raised AttributeError

Six registries, four modules. The first three defined no _missing_ at all, so aenum's Flag machinery composed a pseudo-member for any integer; Flags(-65536) read back as no flags set. The last three reached value.upper() before checking the type.

After the change all 123 of 123 registries raise a bare ValueError for Cls(-1).

Two modules are text-level outliers that are deliberately left alone, because their observable behaviour is already correct: pcapkit/const/ipv6/extension_header.py:20 (ExtensionHeader) and pcapkit/const/ftp/command.py:66 (ConformanceRequirement) define no _missing_ and aenum raises the bare ValueError for them. Adding a redundant guard would be churn.

The fix is in the templates, not the tree

pcapkit/vendor/default.py needed no change — the 113 it emits were already right. The guard lives in four bespoke templates, one per unguarded registry:

  • pcapkit/vendor/tcp/flags.py — its LINE emitted no _missing_, and its FLAG checker was declared but never interpolated. It read isinstance(value, int) and 4 <= value <= 15: those are the registry's bit offsets, while the members it generates are 1 << offset. Emitting it unchanged would have rejected every composite, every member above bit 3, and Flags(0) — so it now reads 0 <= value <= 0xFFFF, the 16-bit field those bits live in.
  • pcapkit/vendor/ftp/command.py — a guard for CommandType (0 <= value <= 0x07) and a type guard for FEATCode and Command.
  • pcapkit/vendor/http/method.py — a type guard for Method.
  • pcapkit/vendor/reg/apptype.py — a guard for TransportProtocol, whose bound is read off cls.__members__ rather than written down, because TransportProtocol.get extends the registry at runtime at max * 2; a literal 0x0F would reject the very member it had just grown. Pinned by test_transport_protocol_can_still_be_extended_at_runtime.

The flag guards end in return super()._missing_(value), the shape #632 landed for the Mobility Header registries, so in-range composites still decompose and #623's unbounded recursion stays fixed.

Then regenerated with the real crawler, python -m pcapkit.vendor tcp.flags ftp.command http.method reg.apptype, exit 0, no warnings.

The rest of pcapkit/const/ is byte-identical

git diff --numstat over the regenerated tree is 42 insertions and 0 deletions, in exactly 4 of the 134 files:

16      0       pcapkit/const/ftp/command.py
2       0       pcapkit/const/http/method.py
12      0       pcapkit/const/reg/apptype.py
12      0       pcapkit/const/tcp/flags.py

Zero deletions is the proof: the live IANA registries still render the committed text character for character, including the 30k-line reg/apptype.py. CI's follow-up pass, isort -l100 -ppcapkit pcapkit/const/*/*.py, exits 0 and changes nothing. Nothing was stale before this change.

The register fallback is preserved

The owner's one named exception. Unchanged and now pinned by ConstEnumRegisterFallbackTests:

Tests

New: tests/const/test_const_enum_builtin_parity.py — 40 tests over the whole tier, including the direct built-in-equivalence assertion the owner's direction is really about (test_the_exception_type_matches_the_built_in_enum: a const registry and a stdlib enum.IntEnum must raise the same type for the same invalid value), a test that EnumError is specifically not what is raised, the register-fallback pins above, and a character-for-character render of pcapkit/vendor/tcp/flags.py against the committed module so a regeneration cannot revert the guard.

One nuance recorded honestly in the test rather than glossed: enum.IntFlag defaults to boundary=KEEP and composes an out-of-range value rather than raising, so the flag registries' parity is against the built-in's own reject mode, boundary=STRICT — same exception type either way, which is the property at issue.

Failing without the fix, passing with it, exit codes read from files rather than from a pipeline:

WITHOUT the fix (pre-fix const modules, new tests):  exit 1
  59 failed, 21 passed, 611 subtests passed
  SUBFAILED(enum='pcapkit.const.tcp.flags.Flags', before='returned 65520, the OR of every declared TCP header flag')
  SUBFAILED(enum='pcapkit.const.ftp.command.CommandType', before='returned 7, the OR of A|P|S')
  SUBFAILED(enum='pcapkit.const.reg.apptype.TransportProtocol', before='returned 15, the OR of tcp|udp|sctp|dccp')
  SUBFAILED(enum='pcapkit.const.ftp.command.Command', before="raised AttributeError: 'int' object has no attribute 'upper'")
  SUBFAILED(enum='pcapkit.const.ftp.command.FEATCode', before="raised AttributeError: ...")
  SUBFAILED(enum='pcapkit.const.http.method.Method', before="raised AttributeError: ...")

WITH the fix:                                        exit 0
  40 passed, 798 subtests passed

One existing pin moved, and it is the sweep this change belongs in: tests/const/test_const_enum_get.py drops Flags from EXPECTED_TO_RESOLVE_ANYTHING and covered goes 110 → 111, because a registry that bounds its domain now has a failure for default to fall back from. EXPECTED_FAILURES in tests/protocols/test_option_roundtrip_unit.py was imported rather than grepped (it is built with ** unpacking): 45 entries, none moved.

Coverage rises on every changed file:

file before after
pcapkit/const/tcp/flags.py 73% 100%
pcapkit/const/ftp/command.py 87% 97%
pcapkit/const/http/method.py 81% 96%

tests/vendor/ 55 passed, tests/protocols/test_option_roundtrip_unit.py + tests/foundation/registry/ 15 passed / 438 subtests, all exit 0. The *_runtime.py failures in tests/protocols/transport/ and tests/protocols/application/ are all FileNotFoundError: sample capture … not found for uncommitted generated fixtures — pre-existing and unrelated (zero non-FileNotFoundError failures in those runs).

Why not breaking

Considered and rejected. Every input whose result changes is an input that cannot occur:

  • The values that now raise are negative or wider than the field — the TCP flags field is 16 bits unsigned, so -1, -65536, 65536 and 1<<70 are not representable on the wire.
  • The library's own callers are unaffected, measured rather than assumed: pcapkit/protocols/transport/tcp.py constructs only Flags(0) and composites of defined bits, and every one of those still resolves.
  • For the three StrEnum registries the change is AttributeErrorValueError, i.e. a crash becomes the documented rejection. Catching AttributeError out of an enum lookup is not a contract.
  • get(key, default) callers gain behaviour rather than losing it: the guard's ValueError is what makes the default fall back, where before a junk pseudo-member was returned.
  • It is convergence onto what 113 of 117 modules already did, not a new contract.

Labels: fix, test.

Out of scope, noted not touched

The generated get() methods carry default: 'int' = -1 and compare it with == rather than is, dispatched on across module boundaries at pcapkit/const/l2tpv2.py:237 and pcapkit/const/ospf.py:191. That is a behaviour change of its own and is left alone here.

Also left alone: docs/source/changelog/1.5.0.rst:1123 and CHANGELOG.md:81 assert in prose that pcapkit/const/tcp/flags.py "defines no _missing_", which this change makes false. No changelog file is touched on this branch by standing rule; the correction goes to #657 with the entry.

Fixes #647

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) test Pull requests that add or correct tests (test: subject prefix) labels Sep 22, 2026
JarryShaw added a commit that referenced this pull request Sep 22, 2026
Records the six constant registries that rejected an invalid value in a way the
built-in `enum` does not -- three composing a pseudo-member for any integer at
all -- and why the issue's own proposal to raise `EnumError` was rejected rather
than adopted.

Also corrects the #623 entry above, which stated that
`pcapkit/const/tcp/flags.py` "defines no `_missing_`". It defined none at the
time and never had #623's recursion defect, which is what that sentence was
about, but it has one now; the tense is fixed and the new entry named.

`CHANGELOG.md` regenerated with `python util/changelog_md.py`;
`--check` exits 0.
…r guard

Of the 117 generated modules under `pcapkit/const/`, 113 reject an invalid
value with a bare `ValueError`, which is what the built-in `enum.IntEnum`
raises. That is the intended behaviour and is left alone; the defect is the
four modules that did something else. Recounted by execution rather than by
grep, at registry rather than module granularity, the divergence is six
registries in those four modules:

- `tcp/flags.py::Flags`, `ftp/command.py::CommandType` and
  `reg/apptype.py::TransportProtocol` are `IntFlag` classes that defined no
  `_missing_` at all, so `aenum`'s `Flag` machinery composed a pseudo-member
  for any integer whatsoever. `Flags(-1)` returned 65520, the OR of every
  declared TCP header flag, so a value no 16-bit wire field can hold read
  back as every flag set at once; `Flags(-65536)` read back as none set.
- `ftp/command.py::Command`, `ftp/command.py::FEATCode` and
  `http/method.py::Method` are `StrEnum` classes whose `_missing_` reached
  `value.upper()` before checking the type, so an integer raised
  `AttributeError: 'int' object has no attribute 'upper'`.

All six now raise a bare `ValueError`. The flag guards hand in-range
composites to `super()._missing_` rather than resolving them, so #623's
unbounded recursion stays fixed. The `_missing_` -> `extend_enum` register
fallback of the mutable registries is untouched and now pinned by test.

`pcapkit/vendor/tcp/flags.py` declared `FLAG = '4 <= value <= 15'` while its
template never interpolated it. Those are the registry's *bit offsets* and the
members are `1 << offset`, so emitting it unchanged would have rejected every
composite; it now reads `0 <= value <= 0xFFFF`, the field those bits live in.
`TransportProtocol` reads its bound off its own members because
`TransportProtocol.get` extends the registry at runtime.

Fixed in the four bespoke templates under `pcapkit/vendor/` and regenerated;
`pcapkit/vendor/default.py` needed no change, since the 113 it emits were
already correct. The regeneration is 42 insertions and 0 deletions across
exactly those 4 of 134 files, so the committed tree was not stale.

`tests/const/test_const_enum_get.py` drops `Flags` from
`EXPECTED_TO_RESOLVE_ANYTHING` and its sweep grows from 110 to 111, since a
registry that bounds its domain has a failure for `default` to fall back from.

tests/const: 40 passed, 798 subtests, exit 0. Coverage of the changed files
rises, measured on the pre-fix tree with the pre-fix tests and on this one:
`const/tcp/flags.py` 73% -> 100%, `const/ftp/command.py` 87% -> 97%,
`const/http/method.py` 81% -> 96%.

Fixes #647
@JarryShaw
JarryShaw force-pushed the fix/const-enum-guard-consistency-647 branch from dc5d17c to 919fa3b Compare September 22, 2026 20:54
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO

Cross-review by an independent subagent on a different model (sonnet) than the one that wrote the change, briefed to falsify rather than bless. It re-derived every load-bearing claim from scratch rather than reusing any script or number from the authoring session, working against immutable git archive snapshots of 919fa3b0a (then dc5d17ce9) and 0c7f2b7c9 rather than a live worktree. Asserted pcapkit.__file__ on every run; reported it as /tmp/xreview-677/tree-dc5d17ce9/pcapkit/__init__.py.

It raised one required fix, and it was right: the commit message quoted the coverage "before" figures as 95/92/90, which were an intermediate measurement of the fixed tree rather than the pre-fix baseline. The correct figures are the ones in the PR body — 73% → 100%, 87% → 97%, 81% → 96% — which the reviewer reproduced exactly and independently. Fixed by amending the commit message; the tree is byte-identical (git diff dc5d17ce9 919fa3b0a is empty), and the branch was re-pushed with --force-with-lease. main was never touched.

What it verified independently, and how:

  • 123 registries, 123/123 raising a bare ValueError for Cls(-1). Its own pkgutil.walk_packages census keyed on aenum.Enum (so flag classes are not skipped). Post-fix: 123 found, 123 raise, type(exc) is ValueError for all 123, none a subclass. Pre-fix: 117 raise, 3 raise AttributeError, 3 resolve — exactly the six named outliers.
  • Exactly six registries changed, and no seventh was missed. A probe matrix of {-1, 0, 1, 1<<70, 'x', None, 1.5, b'x'} plus, for flag registries, a composite of every defined bit and that composite + 1, run over all 123 pre- and post-fix. Exactly 6 registries shifted anywhere in the battery; the other 117 did not move at all.
  • 42 insertions, 0 deletions, 4 of 134 files. Confirmed by plumbing, then went further than the PR did: it re-ran the real crawler against the post-fix tree and diffed the regenerated modules against the committed ones — byte-for-byte identical, exit 0 on all four — and ran isort --check-only over the whole 117-file glob rather than just the four, exit 0.
  • The register fallback. Every named case verified by diffing __members__ before and after, including that AppType.get(65000, proto=tcp) genuinely reaches extend_enum and adds PORT_65000_tcp, and that Method('get') registers nothing.
  • The TransportProtocol dynamic bound, attacked with nine scenarios: repeated extension (bound tracks 15→31→63→127→255), direct extend_enum with a non-power-of-two, extension with lower and negative values, and the specific trap of composite pseudo-members inflating max(cls.__members__.values()). It confirmed composites are not cached into __members__ — including after importing the real apptype.py, whose class body evaluates thousands of | compositions, after which max() is still 8. It could not construct any API sequence that made the guard reject a legitimate value or accept an illegitimate one.
  • Failing without the fix, reconstructed non-destructively (post-fix tree with only the four pcapkit/const/ files replaced by their pre-fix blobs): exit 1, 59 failed — the same 59 the PR reports. Post-fix: exit 0, 40 passed, 798 subtests. It independently reproduced the pytest-9 subtest trap on a minimal example and then confirmed grep -c SUBFAILED is 0 on every clean post-fix run, so nothing is masked.
  • EXPECTED_FAILURES imported rather than grepped: 45 entries, and the roundtrip module passes clean post-fix.
  • The *_runtime.py failures: every one is FileNotFoundError: sample capture … not found, zero non-FileNotFoundError failures.
  • Not breaking: it grepped the whole tree for direct construction of the six registries outside their own modules and found none, and confirmed the .get() call sites at httpv1.py:303, ftp.py:99 and foundation/registry/protocols.py:762 bypass _missing_ entirely, so the guard cannot affect them for any input.
  • 3.10 portability: enum.STRICT/enum.KEEP appear only inside the skipIf(sys.version_info < (3, 11)) method body and nowhere at class-body, decorator-argument or default-parameter level, so a 3.10 collection cannot reach them; enum.StrEnum appears only in docstring prose. Every StrEnum in the changed library files is aenum.StrEnum, the backport.
  • All four changed modules' _missing_ definitions are @classmethod, and the updated prose in both existing test modules is accurate rather than merely plausible.

Two things it flagged without calling them defects, recorded here rather than folded away:

  1. 0xFFF0 would be a defensible alternative to 0xFFFF for the TCP flags bound, since bits 0-3 of that word are the data-offset field. It measured that stdlib enum.IntFlag under its default KEEP boundary also resolves undefined low bits to unnamed pseudo-members, so 0xFFFF is the choice consistent with IntFlag's own semantics — and it confirmed from pcapkit/protocols/transport/tcp.py:458-475 that the schema layer splits the data-offset nibble out before any Flags construction, so bits 0-3 never reach the guard by any real path. A design call, not a defect, but the PR did not spell out the alternative.
  2. Two claims it could not fully verify. 3.10 was checked structurally, not executed — no 3.10 interpreter on this host has the project's dependencies. And it could not reconcile the PR's pre-fix tally of "21 passed, 611 subtests" against its own "34 passed, 745 subtests"; both runs agree on 59 failed. That gap is scope: the PR's without-fix run covered two test files (test_const_enum_builtin_parity.py and test_const_enum_get.py), while the reviewer ran the whole tests/const/ directory, which adds the wholly-passing test_const_enum_lookup.py.

One process note the review surfaced that is worth recording: the worktree was briefly checked out to an unrelated commit mid-review, because the same worktree was being used to stage the changelog entry onto #657's branch via a detached HEAD. The reviewer detected it from the reflog, discarded its first census as invalid, and redid everything against immutable snapshots. It made no modifications itself. The worktree is now clean on fix/const-enum-guard-consistency-647.

CI is not being claimed green: every Actions job on this PR is currently QUEUED. The only SUCCESS is pyup.io/safety-ci, which is a StatusContext rather than an Actions job.

@JarryShaw
JarryShaw merged commit fc32d1b into main Sep 23, 2026
26 checks passed
@JarryShaw
JarryShaw deleted the fix/const-enum-guard-consistency-647 branch September 23, 2026 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix) test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

the generated enum guards raise a bare ValueError in 113 of 117 const modules, and tcp.flags.Flags has no guard at all

1 participant