docs(corekit): state the enum.py rulings as statements, not quotations

[JarryShaw]

• Searched for similar pull requests
• Followed the coding style (docstrings only, no code change)
• tests/corekit passes under plain unittest; prose-only, so no new test case
• Changelog entry: N/A — 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?

• docs — documentation only

Description

Group A of #987 (see also #719): pcapkit/corekit/enum.py only. The 15 reST-emphasised maintainer quotations become statements of the rule, the reason, and the issue where it was settled (#877, #842, #775, #923). No "verbatim" added; the one remaining use (copied verbatim at the LINE template) is about code, not a quotation.

Sources: the two uncited register_alias passages trace to a #842 comment; the range-validation site inherits #877; "always exist on the const enums" inherits #842. The #775 ruling was first made in review of #771 and the #923 ruling in review of #921; both are cited by the issue that carries them.

Verification: ast.parse OK; token streams (strings masked) identical to origin/main; no *" left; tests/corekit 400 tests, 5 failures (documented purge_modules limitation), 16 skipped; test_conventions_doc_claims 38 tests, 1 skip.

[JarryShaw]

NEEDS CHANGES at 3193ed467 — opus cross-review. The blocker is the alias passage at pcapkit/corekit/enum.py:582-584, and it is wrong twice over.

It asserts that AppType and the concrete enumerations are an exception which must call register_alias for members that do not exist yet. No such exception exists — both implementations refuse, and AppType refuses harder:

• base, enum.py:615-618 — raises when the value is in no member map;
• AppType.register_alias, pcapkit/const/reg/apptype/apptype.py:2868-2871 — raises when __registry__.getlist(port) is empty, and again when __registry__ is None.

Only three register_alias definitions exist repo-wide, and the third is the vendor template emitter rather than a runtime registry. So the existing-member requirement holds on every registry, with AppType enforcing it more strictly, not less.

Second, the passage resolves the open conditional raised on #842 about AppType into a settled fact plus a "must". The code answers that question, so the prose should state what the code does rather than harden the hedge.

Going in with the fix: three lines now exceed 100 characters (:259, :348, :586; the file's prior maximum was 96), "the rule set on" garden-paths at :17 and :48, :489 reads "names" against a single-name signature, and :27 drifted to "bad ruling" where the sense is precedent.

Confirmed otherwise: 14 of the 15 sites are faithful, and the change is prose-only — 703 code tokens identical with every string masked.

The red CI is not this pull request. scapy published 2.8.0, which dissects the SCTP DATA payload by PPID and returns Raw where 2.7.0 returned bytes; all six Engines … (Scapy) legs failed on that one assertion with identical bytes on both sides. Fixed on main as 70136fa71 — rebase to pick it up.

[JarryShaw]

Fix pushed as d2b579e49, rebased onto main at 70136fa71 so it carries the scapy fix and CI should come back green.

The blocker is resolved, and a second reviewer on a different model reached the same conclusion independently: the asserted exception exists nowhere. The passage now states that the existing-member requirement holds on every registry, AppType included, whose override is stricter still — the port must already carry a member of that registry.

Also in: maximum line length back to 96 from 115, restoring main's; the two "the rule set on" garden-paths reworded; and register's prose corrected to the single name its signature takes.

One correction to my own verdict, which was wrong. I listed :27 "bad ruling" → "bad precedent" as a drift. I had not verified it before posting. On main that line carries the maintainer's words quoted verbatim, "bad ruling" among them, and rendering it as "bad precedent" is exactly the de-quotation this issue exists to do. Treating it as drift would have pushed the prose back toward quoting him, against the standing instruction on #918/#719. Nothing was changed there, correctly — and that nit should not have been in the verdict.

Prose-only re-verified by me at the new head: tokenising with every string masked gives identical sequences, 574 tokens. tests/corekit 400 passed, 16 skipped, 658 subtests.

A fresh cross-review is running against d2b579e49; the label goes back to review: pending until it lands.

[JarryShaw]

NEEDS CHANGES at d2b579e49 — opus cross-review, round 2. One blocking defect, and it is the same class as round 1's: a prescriptive clause from #842 de-quoted into an asserted universal.

Two sites say all four methods exist on every const enumeration — :56-58 and :436. I measured it and that is false:

const enum classes : 130
carrying all four  : 127
missing register/register_alias : 3
  pcapkit.const.ftp.command.CommandType               (ftp/command.py:186)
  pcapkit.const.ftp.command.ConformanceRequirement    (ftp/command.py:220)
  pcapkit.const.reg.apptype.apptype.TransportProtocol (reg/apptype/apptype.py:28)

All three are declared class X(EnumLookup, ...) — the bare tier, not the registry tier. So the file contradicts itself: lines 10-19 of the same module docstring explain that EnumLookup exists precisely so a closed set is not handed register/register_alias, and :57 then says all four exist everywhere. :436 is the worse site — no scoping lead-in, and it sits inside EnumLookup, the class deliberately lacking two of the four, using the universal to justify get_all living there.

#842's own close does not support the unqualified reading either: its last comments are the maintainer opting to take option (1) and track the remaining work, after which it closed as done with work still outstanding. The fix is to restore the modal and narrow the noun from "enumeration" to "registry", which is both true and what #842 meant.

Non-blocking, going in with it: :90-91 still reads "caller-specified names" where :487-489 was already corrected to the single name the signature takes; a scope qualifier on :596-597, whose claim that an alias adds a name rather than a member is pre-existing on main but false for AppType (its override calls extend_enum and mints a real member), and now reads worse because the new text names AppType two paragraphs above; and one clause distinguishing #842's AppType exception — which is about routing — from pre-existence, since conflating the two is the likely origin of round 1's error.

Confirmed otherwise, and two measurement disputes resolved. Prose-only holds: the ASTs are identical once docstrings are normalised, all seven changed string tokens are docstrings, zero non-docstring strings moved, and the unmasked-comment comparison matches too. The three token counts that looked inconsistent — 574, 703, 575 — differ only by the ENCODING pseudo-token that tokenize.tokenize prepends and generate_tokens does not; all three agree and none was wrong. The round-1 fix is independently confirmed correct, including that "of that very registry" is load-bearing rather than decorative: TCP, UDP and SCTP hold three distinct registries, and port 716 carries pana in UDP while TCP.register_alias(716, …) raises. Line length 115 → 96, restoring main's. The review also re-derived my retraction on "bad ruling" → "bad precedent" and reached the same conclusion independently.

One gap stated honestly: the reviewer could not reproduce the full tests/corekit figure — the run emitted nothing for eight minutes against a contending session — so it ran the four enum-specific files instead, 130 passed / 112 subtests. Low risk, since AST equality proves no executable code changed.

[JarryShaw]

Correction to my own comment above: the last paragraph is wrong. The tests/corekit figure is verified — the reviewer's run finished after it reported, giving 400 passed, 16 skipped, 6 warnings, 658 subtests passed in 273.22s, matching the claim exactly.

It had looked stalled because pytest -q buffers and writes nothing to its output file until the session ends; the run was slow at 4m33s, not hung. The six warnings are pre-existing and unrelated, from test_fields_misc_packet_context.py:271 and test_fields_numbers_unassigned_enum.py:385.

The blocking finding is unchanged and never depended on this: :56-58 and :436 still assert a universal that 3 of 130 const enum classes falsify.

[JarryShaw]

Fixed and pushed as a901e9e5a. The blocker and all three smaller items.

The universal is gone from both sites, and the fix agent reproduced my measurement independently before writing: 130 const enum classes, 127 carrying all four methods, 3 carrying neither register nor register_alias. Both sites now scope the contract to every const registry rather than every const enumeration, which is what #842 meant and what makes the sentence true — and the module docstring gains a line saying #877's closed sets sit outside that contract, so the paragraph no longer contradicts what lines 10-19 explain.

Also in: register's prose corrected to the single name its signature takes; one clause distinguishing #842's AppType exception, which concerns where an alias is routed, from whether the member being aliased must already exist.

The scoping item turned up something worth stating, because it means the base docstring was wrong about AppType rather than merely vague. The claim that an alias adds a name and not a member — __members__ growing while iteration and _value2member_map_ stay put — is false for AppType, whose override mints a real member through extend_enum. Measured by me: TCP.register_alias takes len(TCP) from 6147 to 6148, with __members__ and _member_names_ growing alike. That claim is pre-existing on main, so it is not a regression, but the new text names AppType two paragraphs above and made it read as covering it. Now scoped to the base, with AppType's override described separately.

Verified by me rather than relayed: token sequences identical with every string masked, all three differing string tokens are docstrings, the AST with docstrings blanked compares equal, and maximum line length stays at 96 — main's. The four enum-specific test files give 130 passed / 112 subtests.

Label back to review: pending; a fresh cross-review is running against a901e9e5a.

[JarryShaw]

NEEDS CHANGES at a901e9e5a — opus cross-review, round 3. The defect is mine, and it is the third repetition of the same class in this pull request.

The parenthetical at :583-585 says #842's AppType exception concerns where an alias is routed and not whether the aliased member must already exist. #842 contains two AppType exceptions, and the second is exactly about pre-existence. Verified by me:

• the routing exception is in const: generalise register_alias from AppType to every pcapkit.const enum #842's body;
• the pre-existence carve-out is in comment 5852772329 (2026-09-27T04:55:30Z), where the answer to whether the generic register_alias requires an existing member is that it should always be for an existing member — unless AppType and the concrete enumerations require calling it on non-existing members.

That unless is a carve-out on precisely the question the parenthetical says the issue is not about. I wrote that instruction. Worse, it is the same comment I had identified correctly in round 1 as the maintainer's open conditional — and then I briefed a worker that the exception was only about routing. main's text was lossy by omission, quoting the first half of that sentence and dropping the unless; my fix converted the omission into an assertion, which is worse than what it replaced.

Note the progression, because it is the useful part: round 1 invented an exception that no registry implements, round 2 flattened a modal into a universal, round 3 denied a hedge exists. Three different ways to lose the same qualifier. The worker is choosing between deleting the clause and rendering the hedge properly, with instructions to overrule me if my reading of #842 is wrong again.

Second, non-blocking and also introduced here: :18-19 and :67-68 both say every generated enumeration under pcapkit.const inherits from EnumRegistry, which the sentence added at :59 now contradicts. All three closed sets are generated and inherit EnumLookup, so "generated" is not an escape hatch. Previously the docstring was merely stale at two points; now it states both sides.

Confirmed otherwise, and more strongly than before. "Every const registry" is exactly right in both directions — the set carrying all four methods and the set of EnumRegistry subclasses are identical, 127 each, with the 3 closed sets the only EnumLookup-only classes. The base half of the alias claim, which nobody had checked, is true: on ReturnCode, EtherType and ErrorCode, register_alias grows __members__ by one while _member_names_, iteration and _value2member_map_ stay put, and the returned object is the canonical member by identity. Prose-only was proven with no token exclusions at all — 753 tokens, STRING masked, NL/NEWLINE/INDENT/DEDENT/COMMENT included — which settles the filter question that made four earlier counts look like disagreements. Line length 96, matching main, zero lines over. No :role: target dropped and none introduced.

[JarryShaw]

Fixed and pushed as 37d4aef78, rebased onto main at 52167e331.

The worker took the harder of the two options I offered and was right to: rather than deleting the clause, the passage now states what #842 settled and what it left open, then answers the open part from behaviour — AppType's override requires the port to already carry a member of that registry. It also named the concrete enumerations, which the comment covers and my instruction had dropped, and it removed "stricter still" rather than qualifying it, since the base tests membership in _value2member_map_ while AppType tests __registry__.getlist(port) — never a strengthening of one predicate.

Both contradicting lines now read "generated registry". The three closed sets are themselves generated, so "generated" was never an escape hatch for the claim that every generated pcapkit.const enumeration inherits from EnumRegistry.

I also sent it back once more before committing, for something small that matters for consistency: the fix had left a 52-column line whose successor fit in the remaining space. That is the same defect #992 was sent back for an hour earlier, and the check that catches it is not the maximum line length — which was unchanged and therefore silent — but whether a short line's successor actually fails to fit. The paragraph is now 75-82 columns with only its final line short.

Verified by me at the strictest setting available: 753 tokens identical with no exclusions at all — NL, NEWLINE, INDENT, DEDENT and COMMENT all included, only strings masked. That form makes the filter question moot, and it is what retroactively reconciled the four earlier counts on this file that had looked like disagreements. Both differing string tokens are docstrings, the AST with docstrings blanked compares equal, maximum line length stays 96 with the only line over 95 unchanged at :101, and the four enum test files give 130 passed / 112 subtests.

Label stays review: needs-changes until the new head has its own verdict; a round-4 review is running against 37d4aef78.

[JarryShaw]

GOOD TO GO at 37d4aef78 — opus cross-review, round 4. No blocking finding: this is the first of four rounds that renders #842's hedge rather than deleting, flattening or denying it. The review read the issue before the diff and confirmed the ruling is stated, its residual left open, and current behaviour reported as a separate fact, with no exception invented.

I am holding the label at review: needs-changes anyway and spending one more round, because two of its six non-blocking findings are the same qualifier-firming class this pull request has already repeated three times, and granting them would be the wrong precedent for exactly the issue #987 exists to close.

:68 is still false. It says every generated registry inherits the four methods from EnumRegistry. I measured the resolution order: AppType and all four of TCP, UDP, SCTP, DCCP resolve all four to AppType's overrides. Changing "enumeration" to "registry" fixed the other falsity — the three EnumLookup-only closed sets — and left this one. The next two bullets do say so, which is why it did not block, but the sentence over-claims in isolation.

"leaving open only whether…" is accurate against the pre-existence ruling, whose sole residual is that clause, and false against #842 as a whole — its body carries five open design questions, which I counted. Dropping "only" costs nothing.

"#842 settled on…" may or may not be too firm, and I have asked for it to be checked rather than changed: the tentative phrasing came first, but a later comment restates the rule flatly and the issue closed on a ruling. If it genuinely settled, "settled on" is right and a hedge added there would be the same error pointing the other way.

Three new positive claims were each independently derived and all hold, including one I had not thought to ask for: "of that very registry" is earned — SCTP.register_alias(1, …) raises even though TCP carries port 1, so the predicate really is per-registry rather than per-port-globally.

Two findings I am deliberately not fixing here, with reasons. :func:~aenum.extend_enum`` resolves to nothing — conf.py:101-104 documents that `aenum` serves zero `py:` objects — but `main`'s `enum.py` already carries five identical references and no `nitpicky` is set, so it is pre-existing precedent rather than a new defect class, and it belongs in a change that fixes all six. And the tier-2 bullet's claim that the sub-base routes all four through `_dispatch` is wrong for `register` and `register_alias`, which is pre-existing, untouched here, and not a quotation removal.

Verified mechanically: 753 tokens identical with no exclusions at all, all seven differing string tokens are docstrings on both sides, raw ast.dump differs while the docstring-blanked dump compares equal, line length 96 matching main, and the re-flow moved no wording — a word-level diff of the module docstring gives 747 words either side with exactly two opcodes, both enumeration to registry.

[JarryShaw]

Fixed and pushed as e5836e8dc, rebased onto main at 11c0c2173 now that #992 has merged.

My measurement in the last brief was incomplete, and the worker caught it. I said five registries do not inherit the four methods from EnumRegistry — the AppType family. Measured across all 127: 118 take every method from the base tiers, 5 are the AppType family overriding all four, and 4 carry a hand-written get — Command and FEATCode in const/ftp/command.py, Method in const/http/method.py, OptionType in const/pcapng/option_type.py. So the exception set is nine, and had the fix carved out only AppType the sentence would still have been false for the other four. I verified all three groups myself.

The bullet now excepts the overrides listed below it and the hand-written get overrides, rather than asserting a universal with nine counter-examples. "only" is gone from the #842 residual.

"settled on" stays, checked rather than softened. The first statement of the rule is tentative, but 5852780815 records the contract outright, 5855971633 and 5856019103 restate it flatly, and the issue closed as completed. Adding a hedge where the record is firm would be the same error pointing the other way — which is the trap in over-correcting a pull request that has lost a qualifier three times.

Two things deliberately left for a change of their own, both pre-existing and both verified: get and get_all are defined on EnumLookup rather than EnumRegistry, so "inherits them from here" is true only transitively; and the tier-2 bullet says the sub-base routes all four through _dispatch where only get and get_all do.

Verified by me: 753 tokens identical with no exclusions at all, both differing string tokens are docstrings, the AST with docstrings blanked compares equal while the raw dump differs, maximum line length stays 96, and both edited paragraphs were re-wrapped so no line has an absorbable successor — the worker reported its first pass leaving one and fixing it before reporting.

Label stays review: needs-changes until the new head has its own verdict; a review is running against e5836e8dc.

[JarryShaw]

GOOD TO GO at e5836e8dc — opus cross-review, round 5. No blocking finding. It corrected one claim of mine and found something four rounds of checks had structurally missed, so those first.

My corroboration for keeping "settled on" was partly wrong, and I published it. I cited 5855971633 and 5856019103 as both restating the rule flatly. 5855971633 does not — I re-read it, and it is about the abstraction ruling: the four methods always existing on const enums and moving to the base class. That is the ruling round 2 was about, not alias pre-existence. Two different #842 rulings conflated, by me. Separately, 5852780815 reads as the reviewer voice rather than the maintainer's, so it is a transcription of the ruling and adds no independent weight.

The conclusion survives on better grounds, which the review supplied: the record holds one maintainer statement of the rule, shaped "X unless Y". The docstring renders it as settled on X with Y left open — mapping the hedge onto the residual rather than losing it. The uncertainty is specifically about Y, so softening "settled on" would hedge twice over one uncertainty. Corroborated by 5856019103's flat definition and the issue closing as completed. Keep it.

"Line length verified" has been passing on a metric that cannot see prose. The file maximum is 96 on both sides — but that is an unchanged import line, and it masks the docstring width: main's widest docstring lines are 85, 84, 83; this head's are 91, 91, 88. I measured both. The two 91-character lines predate round 5, so this pull request widened its prose from 85 to 91 and four rounds of "max 96, matching main" never saw it. No stated rule is broken — CONTRIBUTING.md sets no prose column, pylint allows 120 — so it is non-blocking and goes to #719's sweep rather than a sixth round. But the figure should not be carried forward as "line length verified".

Verified otherwise: the 118 / 5 / 4 census reproduced independently, with a static cross-check that 123 class … (EnumRegistry) plus the four class … (AppType) makes 127. "a few hand-written get overrides" is precise rather than merely adequate — no class outside the AppType family overrides any of the other three methods, so naming get is exactly right. The reviewer also argued against putting a number there, with evidence I had not considered: #842's own body says 105 generated registries where there are now 127, so a count over that population has already gone stale once in the record.

Word-level diff across every docstring gives exactly four changes — the three fixes and nothing else. Prose-only at 754 tokens including ENCODING, which is my 753 plus one; sequences identical, all seven differing string tokens are docstrings on both sides, raw ast.dump differs while the blanked dump compares equal, and no line has an absorbable successor.

Labelling review: good-to-go. Unpublished and unmerged — yours to merge, and it closes #987's group A.

[JarryShaw]

Addendum to the verdict above — two non-blocking notes from the review's final pass that my comment did not carry. The verdict is unchanged.

The bullet rewrite also dropped the word "generated" — "Every generated registry" became "The registries". I described the change as three fixes and reported four word-level operations, which is accurate but did not itemise this one. It is harmless, and checked: all 127 const registries are crawler-generated anyway, the exception clause covers all nine non-conformers, and the preceding paragraph already separates registries from #877's closed sets. If anything it is more accurate, since it no longer implies a distinction that does not exist in that population.

"the overrides below" does not formally cover all five AppType-family classes. Bullet 3 states outright that the transport subclasses need no override of their own, so a reader counting overrides "below" finds only bullet 2's — leaving TCP, UDP, SCTP and DCCP apparently inside the inherit-from-here set, which the census says they are not. Bullet 3's own body resolves it two lines later, so the paragraph is self-correcting. Routing to #719's sweep alongside the other two residuals already logged there: get/get_all living on EnumLookup so "from here" is only transitively true, and the docstring prose band drifting from 85 to 91 columns.