fix(const): normalise the key get() looks up, and honour its documented default (#582, #583, #584)

[JarryShaw]

Fixes #582
Fixes #583
Fixes #584

Are these one fix or three?

Three root causes, delivered as one PR, because two of them are the same defect in two copies of one generated template and the third shares the same file set.

#	Root cause	Where
#582	membership tested with the raw key, member registered under key.upper()	pcapkit/const/ftp/command.py:297-298 (+ _missing_ at :310)
#583 item 1	the identical defect, second copy of the same template	pcapkit/const/http/method.py:175-176 (+ _missing_ at :187)
#583 item 2	unanchored _RE_METHOD, and the whole token passed instead of the captured group	pcapkit/protocols/application/httpv1.py:57 and :292
#584	integer path returns Enum(key) directly, so default is never consulted	pcapkit/vendor/default.py:86-87 → 113 generated pcapkit/const/** modules

#582 and #583 item 1 are one fix applied twice — the string-keyed template is duplicated in pcapkit/vendor/ftp/command.py and pcapkit/vendor/http/method.py. #583 item 2 is an independent parse defect. #584 is a third, in the other (integer-keyed) template.

Yes, these are generated - which is why the vendor templates are in the diff

The fix went into the vendor templates as well as the generated tree. pcapkit/const/** is generated from pcapkit/vendor/**, so patching only the committed tree would have left the next python -m pcapkit.vendor run to silently revert all of it. Both sides were changed and a test now renders the template and compares it, character for character, against the module generated from it.

The two vendor templates NOT in this diff, and why they are already correct

13 vendor modules define their own get(); 11 are in this diff. The two that are not:

template	status	why
pcapkit/vendor/pcapng/option_type.py	already correct	integer path never raises
pcapkit/vendor/reg/apptype.py	already correct	integer path never raises

Measured, not assumed - every integer I probed resolves, so there is no raise for a default to rescue:

OptionType.get(1<<70) -> <OptionType.opt_unknown: 1180591620717411303424>
OptionType.get(-1)    -> <OptionType.opt_unknown: -1>
OptionType.get(2**32) -> <OptionType.opt_unknown: 4294967296>
AppType.get(1<<70)    -> <AppType.unknown: 1180591620717411303424 [undefined]>
AppType.get(99999)    -> <AppType.unknown: 99999 [undefined]>

Both reach that through their own bespoke extend_enum fallback rather than the shared Enum(key) call, so the #584 defect - the integer path raises while default is dropped - simply is not present. reg/apptype.py also carries TransportProtocol.get(key), whose signature has no default parameter at all, so the rewrite would have produced a NameError there; that is the one the script's signature check caught.

Crucially, neither file is touched by this PR - the diff's file list contains neither - so there is nothing in them for a regeneration to revert. Their templates and generated modules agree as they stand: rendering each template's own LINE lambda offline reproduces the generated get() blocks character for character (pcapng OptionType's single block, and both of reg/apptype.py's).

One honest caveat on default in those two: it is still unused on their integer paths, so the documentation imprecision #584 complains about persists there. It is not the same bug, because no caller is ever denied a fallback - the lookup always succeeds. Left alone deliberately, and AppType.get especially: it is the hottest of the set (#575's profiling measured 111 extend_enum calls in a single extraction) and sits on the per-packet path via pcapkit/protocols/transport/transport.py:165, so it deserves its own change with its own performance measurement rather than a drive-by edit.

On regenerating

Direct answer: no, I did not verify byte-identical regeneration, because I could not run the crawlers. The detail is below rather than a claim that it passes.

I could not run python -m pcapkit.vendor to prove the 113 generated modules come back byte-identical: the crawlers fetch IANA and Wikipedia over the network, and several are known-broken (#518 records 4 Wikipedia 403s and 3 pointing at a dead IETF URL), so a run would fail for reasons unrelated to this change and could rewrite modules from newer registry data. Saying that plainly rather than claiming a check I did not perform.

What I did instead, offline and for all 13 templates with a get(): render or expand each template and compare its get() block against the module generated from it. All 13 agree. For pcapkit/vendor/default.py this is also pinned as a test (test_the_vendor_template_still_emits_the_fix), which calls the real LINE lambda and asserts equality with pcapkit/const/arp/hardware.py - so a regeneration that dropped the fix would turn the suite red rather than pass quietly.

#582 / #583 item 1 — the key that was looked up was not the key that was registered

if key not in Command._member_map_:                      # raw key
    return extend_enum(Command, key.upper(), ...)        # upper-cased name

'retr' misses the map (which holds RETR), so extend_enum is called for a name that already exists:

>>> Command.get('retr')
TypeError: 'RETR' already in use as <Command.RETR: Retrieve>

Wire-reachable. pcapkit/protocols/application/ftp.py:36 compiles the request pattern with re.I and ftp.py:99 passes the match verbatim, and RFC 959 section 5.3 makes FTP commands case-insensitive — so retr file.txt was a valid request this library could not parse.

Both get() and _missing_ now resolve under the canonical upper-case name. _missing_ carried the identical mismatch and was fixed with it: Command('retr') raised the same TypeError before.

Resolving to the existing member, rather than registering a second one beside it, is load-bearing beyond not crashing — a duplicate GET would carry neither the safe nor the idempotent attribute of the real member.

#583 item 2 — both halves, because either alone is still wrong

_RE_METHOD was unanchored and re.match anchors only at the start:

b'GET' -> b'GET'    b'Get' -> b'G'    b'get' -> None

So b'Get' made match1 truthy on one character, the request-line guard passed, and then the whole mixed-case token went to Method.get.

Fixing one half alone leaves a wrong answer, exactly as #583 says: normalising Method.get alone parses b'Get' as GET off a one-character match; passing the captured group alone parses it as a method named G. Both are in this PR. The pattern is anchored with \Z and match1.group('method') is what is looked up.

_RE_STATUS too — found by auditing _RE_METHOD's siblings

_RE_STATUS = re.compile(rb'\d{3}') (httpv1.py:64) carried the same unanchored-prefix defect, in the same function, and it escaped as the wrong exception type. That pattern is only a guard — the value comes from int(para2) on the raw token — so a prefix match let a malformed status past the guard and then out of int() uncaught, where _read_http_header documents Raises: ProtocolError. Measured on the pre-fix tree:

b'HTTP/1.1 200x OK' -> ValueError: invalid literal for int() with base 10: b'200x'
b'HTTP/1.1 2000 OK' -> ValueError: 2000 is not a valid StatusCode

Both are now ProtocolError. RFC 9110 section 15 gives status-code = 3DIGIT, exactly three, so the anchor is what the grammar already said. _RE_VERSION was audited at the same time and is safe as it stands, because both of its call sites read the captured group rather than the raw token.

No re.I was added, deliberately. Method tokens are case-sensitive per RFC 9110 section 9.1, so Get is not GET; it is now a malformed request line (ProtocolError) rather than a mis-parse. Measured after the fix:

b'GET / HTTP/1.1\r\n...'  -> (Type.REQUEST, <Method.GET>)
b'Get / HTTP/1.1\r\n...'  -> ProtocolError: HTTP: invalid format
b'get / HTTP/1.1\r\n...'  -> ProtocolError: HTTP: invalid format
b'HTTP/1.1 404 Not Found' -> (Type.RESPONSE, ...)          # response path untouched

#584 — the sweep the issue asked for, and what "placeholder" means

get() delegated to the enum call, and _missing_ cannot see the caller's default. The integer path now consults it:

if isinstance(key, int):
    try:
        return Hardware(key)
    except ValueError:
        if default == -1:
            raise
        return Hardware(default)

-1 is the placeholder the generated signature already carried, and it is what separates no default was supplied from a default was supplied and should be used. Keeping it means no existing caller changes behaviour — nothing in the library or the test suite passes a positional default to an integer-keyed get() — and a caller that asked for no fallback still gets the error rather than a silent substitution. That guard has its own test, so a future "fix" cannot quietly turn every failed lookup into a fallback.

One thing to read carefully: #584's literal repro still raises, and that is the correct outcome. The default is now consulted; 'X' is simply not a valid default for an enum whose signature says default: 'int'. What changed is which value is rejected, which is the observable proof that the default is reached at all:

before:  Hardware.get(99999, 'X') -> ValueError: 99999 is not a valid Hardware
after:   Hardware.get(99999, 'X') -> ValueError: 'X' is not a valid Hardware
         Hardware.get(99999, 0)   -> <Hardware.Reserved_0: 0>
         Hardware.get(99999)      -> ValueError: 99999 is not a valid Hardware   (unchanged)

Scope, measured rather than assumed. #584 said "worth a sweep before fixing" and named Hardware, Operation and LinkType with only Hardware verified. Sweeping all 118 IntEnum/IntFlag registries under pcapkit/const:

• 110 raised on an out-of-range integer while dropping default — the defect was live in all of them, and all 110 are fixed and asserted.
• 3 never raise (ProtectionAuthority, CGAType, tcp Flags) — they auto-extend the whole integer space, so there is nothing for a default to supply.
• 5 carry no get(key, default) at all. reg TransportProtocol is the interesting one: its get has an integer path of exactly the shape being rewritten but no default parameter, so a blind pattern rewrite would have produced a NameError. The rewrite checked the signature, not the body.

Two registries were deliberately left alone: pcapng OptionType and reg AppType. Both already have a bespoke integer fallback that resolves every value, so neither drops a default by raising, and AppType.get is on the per-packet path via pcapkit/protocols/transport/transport.py:165. #584's unverified AppType double-extend claim is therefore not addressed here — see below.

Not reachable from wire data, as #584 says: Hardware(40) returns Unassigned_40, so every value a 16-bit field can carry already resolves. This is a contract fix, not a parse fix.

A second citation correction

The first draft of the _RE_STATUS fix cited RFC 9110 section 15 for status-code = 3DIGIT. Wrong: that production is not in RFC 9110 at all. It is RFC 9112 section 4 ("Status Line"), because status-code is part of HTTP/1.1's status-line = HTTP-version SP status-code SP [ reason-phrase ]; RFC 9110 section 15 covers what the codes mean and the IANA registry.

Verified against the primary sources rather than accepted: fetched both RFCs and grepped them. status-code = 3DIGIT appears at RFC 9112 line 673, inside section 4 (heading at 645, section 5 at 695), and again in its collected ABNF at line 2106. It appears nowhere in RFC 9110. RFC 9112 settles the division itself, in the paragraph immediately after the production: "HTTP's core status codes are defined in Section 15 of [HTTP], along with the classes of status codes, considerations for the definition of new status codes, and the IANA registry for collecting such definitions."

Corrected in three places — the changelog bullet (and so the regenerated CHANGELOG.md), the comment above _RE_STATUS in pcapkit/protocols/application/httpv1.py, and the test_httpv1_status_regex_is_anchored docstring.

Deliberately not blanket-replaced, for the same reason as the RFC 959 fix:

• The two :rfc:9110#section-9.1`` citations for method case-sensitivity are correct and untouched — that rule genuinely is RFC 9110.
• The ~46 :rfc:9110#section-15.x`` citations in pcapkit/const/http/status_code.py are correct and untouched — those are per-code semantics, which is exactly what section 15 is for.

Both citation errors in this PR came from the issue text and were caught by cross-review rather than by me; recording that rather than quietly fixing them.

Evidence: failing before, passing after

Each new test run alone, verdict taken from the process exit code written to a file — this pytest has no pytest-subtests, so a failing subtest's parent still prints PASSED.

Against the unfixed tree (sources reverted, new tests in place):

FAIL   exit=1  test_the_reported_case_returns_the_default
PASS   exit=0  test_the_placeholder_still_raises
FAIL   exit=1  test_the_two_unverified_enums_from_the_issue
FAIL   exit=1  test_every_integer_path_consults_the_default
FAIL   exit=1  test_the_vendor_template_still_emits_the_fix
PASS   exit=0  test_the_sweep_size_is_pinned
FAIL   exit=1  test_command_get_is_case_insensitive
FAIL   exit=1  test_ftp_read_parses_a_lowercase_request
FAIL   exit=1  test_method_get_is_case_insensitive
FAIL   exit=1  test_httpv1_method_regex_is_anchored
FAIL   exit=1  test_httpv1_read_header_uses_the_captured_method

Underlying failures, verbatim: TypeError: 'RETR' already in use as <Command.RETR: Retrieve>, TypeError: 'USER' already in use as <Command.USER: User Name>, TypeError: 'GET' already in use as <Method.GET>, ValueError: 99999 is not a valid Hardware, AssertionError: b'G' != None, AssertionError: b'GET' != None.

With the fix:

PASS   exit=0  test_the_reported_case_returns_the_default
PASS   exit=0  test_the_placeholder_still_raises
PASS   exit=0  test_the_two_unverified_enums_from_the_issue
PASS   exit=0  test_every_integer_path_consults_the_default
PASS   exit=0  test_the_vendor_template_still_emits_the_fix
PASS   exit=0  test_the_sweep_size_is_pinned
PASS   exit=0  test_command_get_is_case_insensitive
PASS   exit=0  test_ftp_read_parses_a_lowercase_request
PASS   exit=0  test_method_get_is_case_insensitive
PASS   exit=0  test_httpv1_method_regex_is_anchored
PASS   exit=0  test_httpv1_read_header_uses_the_captured_method

10 of 12 fail without the fix. The two that pass either way are the ones that should: test_the_placeholder_still_raises pins behaviour the fix must preserve, and test_the_sweep_size_is_pinned is a structural pin.

Coverage

Same scoped run, before and after (coverage run, never pytest-cov). Every changed file rose, despite each gaining 4-5 statements:

file	before	after
pcapkit/const/arp/hardware.py	69%	90%
pcapkit/const/arp/operation.py	65%	80%
pcapkit/const/ftp/command.py	95%	96%
pcapkit/const/http/method.py	94%	96%
pcapkit/const/reg/linktype.py	94%	97%
pcapkit/protocols/application/ftp.py	100%	100%
pcapkit/protocols/application/httpv1.py	100%	100%

Regression runs

All scoped, never the whole tree. Exit codes read from files.

suite	result
tests/test_docstring_contract.py + tests/const/	20 passed, 238 subtests
tests/protocols/test_option_roundtrip_unit.py + tests/project/	57 passed, 827 subtests
tests/protocols/application/ + transport/	176 passed, 112 subtests
tests/protocols/internet/ + misc/ + schema/	278 passed, 1 skipped, 838 subtests

Two worth calling out:

• EXPECTED_FAILURES is untouched. No entry started passing, so none is deleted. Its 46 entries were read by importing the table (it is built with ** unpacking and cannot be grepped); only one concerns HTTP at all, httpv2-frame/PRIORITY, and it is unrelated.
• test_docstring_contract.py passes despite 113 generated docstrings changing. It walks every function under pcapkit/ and cross-checks raise against documented Raises:; the new bare raise needed no clause, and the default: continuation lines sit at entry-depth + 4 so they are not mistaken for swallowed section headers. The KNOWN_DEFECTS entries in the four pcapkit/vendor/mh/*_flag.py files and pcapkit/vendor/pcapng/option_type.py are still defects — only the template strings in those files were edited, not their context() docstrings.

Changelog: three bullets in docs/source/changelog/1.5.0.rst, CHANGELOG.md regenerated with python util/changelog_md.py, and --check exits 0.

CI state

The one red check this PR ever had was not this change, and it is now fixed upstream.

Python 3.14 (the full-tree unit suite) failed twice on a single test:

FAILED tests/utilities/test_stacklevel.py::StacklevelAttributionTests::test_a_truncated_traceback_limit_does_not_blind_the_walk
AssertionError: 2 != 1 : ["unclosed file <_io.BufferedReader name='.../examples/captures/in.pcap'>",
                          'Probe: info class has been finalised; now skipping']

The assertion at tests/utilities/test_stacklevel.py:304 expects exactly one captured warning and got two: the intended probe, plus a stray ResourceWarning for an unclosed examples/captures/in.pcap handle that the garbage collector finalised inside that test's catch_warnings block. Both times it was the only failure out of 1186 passed and 2858 subtests.

Established as a pre-existing, order- and GC-dependent flake rather than assumed:

• No overlap. This diff touches pcapkit/const/**, pcapkit/vendor/**, pcapkit/protocols/application/httpv1.py and three test files. None opens in.pcap; none touches file handles, the warning machinery, stacklevel or sys.tracebacklimit. The one apparent hit — stacklevel=2 in pcapkit/vendor/default.py — is at lines 520-601, whereas this change to that file is confined to the LINE template string at lines 79-97. tests/utilities/test_stacklevel.py is not in the diff at all.
• The same suite passed on Python 3.12, 3.13 and 3.15 in the same runs; only 3.14 tripped.
• It passes locally on 3.14 in isolation: 16 passed, 65 subtests, exit code 0.
• The test documents its own order-sensitivity — its docstring notes the defect it guards "only ever reproduced in a full-suite run", which is exactly the condition a stray ResourceWarning needs.

It was filed as #606 and has since been fixed and merged as 4529fdb1f, which is in this PR's base after the last rebase. It was never papered over here: no filterwarnings, no retry decorator, and no edit to that test in this PR.

Found and deliberately NOT fixed

1. _missing_ recursion in the four Mobility Header flag registries. pcapkit/const/mh/binding_ack_flag.py:73, binding_update_flag.py:88, handover_ack_flag.py:61, handover_initiate_flag.py:64 each end _missing_ with return cls(value), which re-enters _missing_ forever for any in-range value that is not a defined combination. Measured on the pristine HEAD copy, so it is pre-existing and not introduced here: BindingACKFlag.get(0x01) and get(0x99) both give RecursionError: maximum recursion depth exceeded, while get(0x04) returns <BindingACKFlag.S: 4> — bit 0x01 is simply not a defined member.

   Not wire-reachable, verified by building real BU/BA/HI/HAck messages with the flags octet set to 0x01 and to 0xFF and parsing them through MH(io.BytesIO(raw), len(raw), extension=True): all four parse cleanly. pcapkit/protocols/schema/internet/mh.py:2610,2639,2792,2821 decode those octets with BitField over a per-named-bit namespace, which reads only the recognised bits and discards the rest, so these four enums are dead imports as far as the live parser is concerned. pcapng RecordType and SecretsType end with the same line but are safe, because extend_enum registers the member first. No test references any of the four class names.

   Left out because it is a distinct defect in _missing_ rather than get(), is not named by any of the three issues, and needs its own decision about what an undefined flag combination should return. Worth its own issue.

2. Method.get silently duplicates the two hyphenated methods. Method.BASELINE_CONTROL has the name BASELINE_CONTROL and the value BASELINE-CONTROL, so the real wire token misses _member_map_ and Method.get('BASELINE-CONTROL') registers a second member named BASELINE-CONTROL carrying neither safe nor idempotent. Same for VERSION-CONTROL. This is name mangling at generation time (- → _), not case normalisation, and behaviour is unchanged by this PR ('BASELINE-CONTROL'.upper() is itself). Left alone because it needs its own decision — should Method.get('BASELINE_CONTROL') work too? — and would otherwise widen this PR.

3. _RE_VERSION is also unanchored (httpv1.py:59), so HTTP/1.1x matches. Method.get raises TypeError on a mixed-case HTTP method, and httpv1.py:292 passes the whole token instead of the captured method #583 names only _RE_METHOD, and the version group is re-read from the match rather than from the raw token, so nothing mis-parses today. Not changed.

4. http/status_code.py's get() discards extend_enum's return and re-looks-up with StatusCode[key]. Harmless — extend_enum registers the member, so the second lookup finds the same object — and untouched beyond the integer-path fix.

5. get()'s documented default is ignored on the integer path across the shared const/ enum template #584's unverified AppType double-extend claim is not addressed, for the reasons above: AppType.get's integer path already has its own fallback, it is on the per-packet path, and the claim was explicitly recorded as unverified.

Cross-review

Reviewed by an independent agent on a different model (Sonnet; this change was authored on Opus).

Final verdict on this head (bf4d0c9f9): GOOD TO GO, after three passes (aab3350a9, 0afb70569, bf4d0c9f9). The first pass, against aab3350a9, returned NEEDS CHANGES with three items — all now addressed, and all three independently re-measured by me rather than taken on the reviewer's word:

1. Stale base / changelog conflict. Correct, and now resolved: rebased onto 0bd517a1c, both sides' bullets kept, CHANGELOG.md regenerated, --check exits 0, and GitHub reports mergeable: MERGEABLE.
2. test_every_integer_path_consults_the_default permanently polluted two shared registries. Correct and confirmed by my own measurement — ProtectionAuthority 8 members to 9, CGAType 7 to 8, and it sticks. The reviewer also rightly noted that for those registries the probe asserted nothing about the fix, since their try never raises. Fixed: the three always-resolving registries moved out of the sweep into test_the_always_resolving_registries_have_nothing_to_fall_back_to, which probes them deliberately, and setUpClass now registers addClassCleanup(purge_modules, ['pcapkit']) so the pollution is dropped by this module rather than incidentally by the next test's setUp. One correction to the reviewer's framing: tcp Flags does not extend — it is an IntFlag and returns a pseudo-member (12 members before and after) — so the shared assertion is "resolves", not "extends".
3. _RE_STATUS undisclosed. Correct, and worse than reported: as well as 200x, b'HTTP/1.1 2000 OK' also escaped as ValueError: 2000 is not a valid StatusCode. Fixed rather than merely disclosed, since it is the same defect class in the same function in a file this PR already changes, and it makes the reader honour its documented Raises: ProtocolError.

Two findings from the review worth recording, neither requiring a change:

• The reviewer found inputs that "parsed" before and are now rejected: b'GET! /x HTTP/1.1' and b'POST-DATA123 /x HTTP/1.1'. Under the old code the unanchored match made match1 truthy on a prefix and the whole token was registered, so these silently corrupted the global Method enum with members like Method.GET!. Rejecting them is strictly better, and it closes an unbounded-enum-growth-from-malformed-input path.
• The reviewer independently re-derived the 118 / 110 / 3 / 5 split and the template-vs-generated equality (including tcp/flags.py, http/status_code.py, ipv6/extension_header.py and all four mh/*_flag.py) and reproduced 8 of the failing-then-passing cases from a scratch tree. It also confirmed via aenum's Enum.__new__ that a _missing_ returning an existing member is not cached into _value2member_map_, so the registry is not warped — which I verified separately: _member_map_ and _value2member_map_ both grow by 0, and list(Command) contains RETR exactly once.

What the re-review verified on the final head

On the third pass it re-fetched both RFC texts itself rather than trusting the quotes, confirmed status-code = hits RFC 9112 at lines 673 and 2106 (both inside section 4) and returns nothing in RFC 9110, checked from the diff that the new citation lands in exactly the three claimed places, and confirmed the two deliberate exclusions really are untouched — including that pcapkit/const/http/status_code.py's only hunk is the integer-path fix, with no citation text altered. It re-ran --check (exit 0) and the scoped suite (45 passed, 138 subtests, exit 0) against the final head, and independently pulled #606 to confirm the CI flake disclosure is accurate.

It re-derived rather than re-read, and could not falsify anything:

• Pollution containment: loaded the test module and ran the whole class in one process via unittest.TextTestRunner, then checked sys.modules and re-imported. addClassCleanup fired, pcapkit.const.ipv4.protection_authority was gone, and a fresh import came back with 8 members (not 9) and CGAType with 7 (not 8), with the class object identity actually different. It also confirmed from aenum's source that _create_pseudo_member_ caches by value in _value2member_map_, which is why Flags.get(1<<70) is Flags.get(1<<70, 0) holds without any extend_enum — my correction to its first-pass framing checked out.
• Not tautological: assertEqual(int(resolved), UNRESOLVABLE) is real content, and because the three get() calls are not wrapped in assertRaises, the test fails loudly if any of the three ever regresses into raising.
• _RE_STATUS: confirmed both malformed statuses now raise ProtocolError and HTTP/1.1 200 OK still parses to 200.
• The citation: fetched raw rfc959.txt itself rather than trusting my report, and independently placed the sentence inside §5.3. It also noted §5.3's own text settles it — "The command functions and semantics are described in the Section on Access Control Commands... The command syntax is specified here."
• The two excluded vendor templates: attacked both claims and could not break either. It confirmed the integer paths never raise (AppType.get(64999) and OptionType.get(64999) both auto-extend, 8182→8183 and 40→41 members), that a deliberately garbage default='not-even-an-int' passed to AppType.get is simply never consulted on that path, and then rendered each file's own LINE lambda with the real FLAG values read from the source rather than guessed and diffed the get() block byte for byte against the generated module — 1072 and 827 characters, identical.

Notes for the reviewer

• 113 of the 132 changed files carry an identical 11-line hunk, applied by a script that refused any get() whose signature lacked default and any integer path that was not the bare return X(key) form. It reported 9 vendor templates and 113 const modules changed and 5 sites skipped, and those 5 skips are exactly the expected ones.
• One commit, rebased onto origin/main at 4529fdb1f. Five rebases were needed as fix(changelog-md): cite a line the entry has, and accept an anchored :rfc: role (#588, #592) #595, fix(tcp): resolve the connection flags before building the options (#587) #597, fix(corekit): recompute _need_process from the width in force, not once from the placeholder (#591) #598, fix(corekit): size the width repair with a real ceiling, not a floored one (#599) #600 and test_stacklevel asserts an exact warning count, so a leaked in.pcap handle fails it on unrelated PRs #606 landed during this work; four of them conflicted on the shared changelog. Every time, all sides' bullets were kept in docs/source/changelog/1.5.0.rst and CHANGELOG.md was discarded and regenerated with python util/changelog_md.py rather than hand-merged, with --check exiting 0 each time. Nothing under pcapkit/ or tests/ ever conflicted.
• fix(changelog-md): cite a line the entry has, and accept an anchored :rfc: role (#588, #592) #595's converter fix is in the base, so the anchored :rfc:`9110#section-9.1``` and :rfc:9110#section-15``` roles now convert properly. One trap hit on the way: the converter rejects a double-backtick literal span that straddles a source line break, which is why the quoted ValueError` text sits on one line.

A citation correction

#582's body, and this PR's first draft, cited RFC 959 section 4.1 for FTP command case-insensitivity. The cross-review caught that as wrong, and it is: the rule is in section 5.3 (COMMANDS), which gives the command syntax - "The command codes are four or fewer alphabetic characters. Upper and lower case alphabetic characters are to be treated identically. Thus, any of the following may represent the retrieve command: RETR Retr retr ReTr rETr". Section 4.1 (FTP COMMANDS) only lists the per-command semantics.

Verified against the RFC text rather than taken on trust: the sentence sits at lines 2560-2572, between the 5.3. COMMANDS heading at 2548 and 5.4. at 2743, while section 4.1 spans 1394-1963. Corrected in the changelog bullet, in the test docstring, and in the commit message.

Worth noting what was not changed: the four existing :rfc:959#section-4.1`` citations in pcapkit/const/ftp/command.py and `pcapkit/vendor/ftp/command.py` are a different claim - the command kind (access control, transfer parameter, service) - and section 4.1 is the right citation for that. A blanket find-and-replace would have broken them.

[JarryShaw]

❌ NEEDS CHANGES — head aab3350a9015294ced9d9c428ec5028bcc09bd12: RFC 959's case-insensitivity rule ("Upper and lower case alphabetic characters are to be treated identically... RETR Retr retr ReTr rETr") is in §5.3 (COMMANDS), not §4.1 (FTP COMMANDS, which only lists individual command semantics) — fix the citation in docs/source/changelog/1.5.0.rst (the "Fixed" bullet), tests/protocols/application/test_ftp_unit.py:71, and the commit message, from "section 4.1" to "section 5.3". Everything else verified correctly; see appendix.

[JarryShaw]

Cross-review appendix — PR #596

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head aab3350a9015294ced9d9c428ec5028bcc09bd12 in an isolated worktree (/tmp/pcapkit-review/pr596, removed after this review). This is a 132-file PR; I verified the mechanism thoroughly, sampled across the sweep's categories, and independently re-derived every RFC citation rather than accepting the PR's reading.

Fixes keywords and CI

closingIssuesReferences = [582, 583, 584], matching the three Fixes lines. CI: statusCheckRollup.state = PENDING; CheckRun tally (GraphQL, __typename == "CheckRun"): 0 FAILURE, 2 SKIPPED (COMPLETED), 20 QUEUED, 1 IN_PROGRESS.

❌ The defect: RFC 959 mis-cited

The PR body, commit message, docs/source/changelog/1.5.0.rst (hence the regenerated CHANGELOG.md), and tests/protocols/application/test_ftp_unit.py:71 all say "RFC 959 section 4.1 makes FTP commands case-insensitive." I fetched rfc-editor.org/rfc/rfc959.txt directly and checked both candidate sections:

• §4.1 "FTP COMMANDS" (line 1394 of the plaintext) — lists individual commands (USER, PASS, RETR, ...) and their semantics. No mention of case at all.
• §5.3 "COMMANDS", under "5. DECLARATIVE SPECIFICATIONS" (line 2547) — reads verbatim: "The commands begin with a command code followed by an argument field. The command codes are four or fewer alphabetic characters. Upper and lower case alphabetic characters are to be treated identically. Thus, any of the following may represent the retrieve command: RETR Retr retr ReTr rETr."

That is the actual textual basis for the claim — and notably the PR's own test (test_command_get_is_case_insensitive) uses the exact same four-way casing example (RETR, retr, ReTr, rEtR) as this passage, which is strong evidence the author read §5.3 and mis-attributed it to §4.1. The underlying behavioral claim (FTP commands are case-insensitive) is correct; only the section number is wrong. This isn't pedantry: it ships into a permanent, user-facing CHANGELOG.md bullet and into git history via the commit message, and this review programme has already had one RFC-citation correction go the other way (a demand for a nonexistent RFC 6554 figure number), so citation accuracy here is explicitly load-bearing.

Checked the counterpart citation and it is correct: RFC 9110 §9.1 "Overview" (line 3728 of rfc-editor.org/rfc/rfc9110.txt) reads "The method token is case-sensitive because it might be used as a gateway to object-based systems with case-sensitive method names. By convention, standardized methods are defined in all-uppercase US-ASCII letters." — exactly matches :rfc:9110#section-9.1`` as used in httpv1.py:59, the changelog, and the test docstring. No issue there.

#582 / #583 item 1 — key normalisation

Directly exercised, not just read:

• Command.get('retr') → Command.RETR (is identity confirmed, no TypeError).
• Command('retr') (the _missing_ path) → same, resolves to the real RETR member.
• Method.get('get') → Method.GET, with safe == True preserved — confirms it resolves to the existing member rather than registering a shadow duplicate lacking safe/idempotent, which is exactly the load-bearing property the PR claims.

#583 item 2 — anchored regex + captured group

pcapkit/protocols/application/httpv1.py:60: _RE_METHOD = re.compile(rb"(?P<method>[A-Z][A-Z-]*)\Z") — confirmed \Z-anchored. Directly tested: b'GET'/b'POST'/b'CONNECT' match in full; b'Get' and b'get' now return None (previously b'Get' prefix-matched to b'G'). httpv1.py:296 now reads match1.group('method'), not the raw para1 token — confirmed by direct read of the surrounding code. No re.I was added, matching the RFC 9110 §9.1 citation verified above.

#584 — default consulted, scope reconciled

Directly exercised: Hardware.get(99999, 'X') raises on the default (ValueError: 'X' is not a valid Hardware), Hardware.get(99999, 0) returns a genuine Hardware.Reserved_0 member (isinstance and repr() confirmed, not a raw int), Hardware.get(99999) (no default given) is unchanged. All three match the PR's claimed before/after table exactly.

Reconciled the file-count claims independently rather than taking them on faith — initially found what looked like a mismatch (9 vendor templates / 113 const modules claimed vs. 11 vendor / 115 const files actually touched by my own git diff --stat), then found the reconciliation: pcapkit/vendor/ftp/command.py and pcapkit/vendor/http/method.py (+ their generated const/ counterparts) belong to the string-keyed #582/#583 fix, not the integer-keyed #584 sweep the "9 templates / 113 modules" figures describe. Subtracting those 2+2 files gives exactly 9 and 113. This checks out.

Spot-checked the skip-list claims by reading the actual code, not the prose:

• pcapkit/const/reg/apptype.py:26-54 — a nested TransportProtocol(IntFlag) class with get(key) taking no default parameter at all — confirmed genuinely untouched in the diff, exactly the "blind rewrite would produce NameError" case the PR names.
• Same file's AppType.get() (line 30585) — confirmed its integer path always falls through to extend_enum(...) on any ValueError from _missing_, so it can never propagate an error to the caller; genuinely untouched, and the "bespoke fallback that resolves every value" claim holds.
• pcapkit/const/pcapng/option_type.py — untouched, has its own get(key, default=-1, *, namespace=...) with a bespoke resolution path (not independently exercised at runtime, but the signature and the presence of a working _missing_ support the claim).
• ProtectionAuthority and CGAType — directly called ProtectionAuthority(99999999) and CGAType(99999999): both resolve without raising, confirming "auto-extend the whole integer space."
• One nuance worth flagging, though I resolved it as consistent rather than a defect: pcapkit/vendor/tcp/flags.py and the generated pcapkit/const/tcp/flags.py were modified (a try/except ValueError was added around Flags(key)), even though the PR lists "tcp Flags" among the "3 never raise" registries in a way that initially read as "left alone." I tested Flags(v) for v in {0, 1, 255, 65535, -1, 100000} and it never raises, so the added except branch is genuinely dead/unreachable code — harmless, but the PR's own framing ("there is nothing for a default to supply") describes the behavior accurately while the diff still touches the file. This is because the rewrite script is described as syntactic (any file matching the if isinstance(key, int): return X(key) shape), which mechanically includes the 3 "never raise" registries too — 110 + 3 = 113, which is exactly the const-module count. Not a defect; noting it because it took an extra check to confirm rather than assume.

Tests fail without the fix, pass with it

Reverted all 127 non-test source files to fa6d18e31 (kept the PR's 3 test files), then ran each of the 11 named tests individually as its own pytest invocation, reading the exit code from $? rather than the aggregate summary — this matters because an aggregate run showed 128 failed purely from SUBFAILED subtests not failing their parent's line (the exact pytest-has-no-pytest-subtests trap this programme has been burned by before). Per-test-process results against the reverted tree:

test	reverted	fixed
test_the_reported_case_returns_the_default	exit 1	exit 0
test_the_placeholder_still_raises	exit 0	exit 0
test_the_two_unverified_enums_from_the_issue	exit 1	exit 0
test_every_integer_path_consults_the_default	exit 1	exit 0
test_the_vendor_template_still_emits_the_fix	exit 1	exit 0
test_the_sweep_size_is_pinned	exit 0	exit 0
test_command_get_is_case_insensitive	exit 1	exit 0
test_ftp_read_parses_a_lowercase_request	exit 1	exit 0
test_method_get_is_case_insensitive	exit 1	exit 0
test_httpv1_method_regex_is_anchored	exit 1	exit 0
test_httpv1_read_header_uses_the_captured_method	exit 1	exit 0

9 of 11 fail without the fix, exactly as claimed, and the two that pass either way (test_the_placeholder_still_raises, test_the_sweep_size_is_pinned) are the ones that should, by design.

Pre-existing bug disclosure (found, not fixed)

Verified independently: BindingACKFlag(0x01) and (0x99) raise RecursionError on both this branch and main — confirmed pre-existing and unchanged, not introduced by this PR. BindingACKFlag(0x04) resolves fine, matching the claim that it's the undefined-combination case that recurses.

Changelog regeneration

python util/changelog_md.py --check exits 0 against the committed tree. Regenerated and diffed against main's CHANGELOG.md: exactly 3 bullets added (matching "three bullets"), no other line touched.

Regression

Scoped run (tests/const/ tests/protocols/application/ tests/protocols/transport/, not the whole tree): first attempt showed 12 failures, all FileNotFoundError: sample capture ... not found — a fresh worktree has no generated fixture captures, exactly the trap flagged for this repo. Ran python examples/generators/make_samples.py, reran: 201 passed, 338 subtests passed, exit 0.

Not independently checked

• pcapng.OptionType's bespoke fallback was read but not exercised at runtime (low risk: untouched by the diff either way).
• The exact coverage percentages in the PR's table were not re-derived (the pass/fail test evidence above is the load-bearing check; coverage deltas are secondary).

Disagreement log

One real, specific defect: the RFC 959 citation (section 4.1 → should be section 5.3), present in the commit message, PR body, changelog bullet, and test docstring. Everything else — the code mechanism for all three issues, the test evidence, the skip/never-raise scoping, the RFC 9110 citation, the pre-existing-bug disclosure, and the changelog regeneration — checked out under independent re-derivation.

[JarryShaw]

Re-pointing my verdict — PR #596

My prior review (verdict + appendix, both at 23:17Z) was pinned to the pre-rebase head aab3350a9015294ced9d9c428ec5028bcc09bd12. The PR has since rebased onto the new base 0bd517a1c (after #595 merged) and now sits at 0b5eff9b853efda73dce85c80a46a80a8c92e96c.

Verified rather than assumed: git diff <old-base>..<old-head> vs. git diff <new-base>..<new-head>, restricted to every file this PR owns except the changelog-gate files that arrive through the rebase (CHANGELOG.md, docs/source/changelog/1.5.0.rst, util/changelog_md.py, tests/project/test_changelog_md.py — all #595's), are byte-identical (3760/3760 lines, diff exit 0). So every finding in my original appendix — the Command.get/Method.get mechanism, the anchored _RE_METHOD regex, the default-consulted integer path, the skip-list/never-raise reconciliation, the 9-of-11 test pass/fail evidence, the pre-existing RecursionError disclosure — carries over unchanged, because the code producing them didn't change.

The one defect I opened NEEDS CHANGES for is also unchanged. Confirmed by fetching the new head directly: docs/source/changelog/1.5.0.rst still reads "and RFC 959 section 4.1 makes FTP commands case-insensitive" (should be §5.3, "COMMANDS" — verified against rfc-editor.org/rfc/rfc959.txt in my original appendix; §4.1 only lists individual command semantics and never discusses case), and tests/protocols/application/test_ftp_unit.py:71 still cites :rfc:959#section-4.1``. The rebase did not touch this.

❌ NEEDS CHANGES — head 0b5eff9b853efda73dce85c80a46a80a8c92e96c: same defect as before — fix the RFC 959 citation from "section 4.1" to "section 5.3" in docs/source/changelog/1.5.0.rst, tests/protocols/application/test_ftp_unit.py:71, and the commit message. Everything else remains verified correct; see my original appendix (comment at 23:17:14Z on this PR) for the full evidence, which stands unchanged.

[JarryShaw]

Re-pointing again — PR #596, head 33692df984bf2c936ff10d5d9718f8e01d24cd21

This is not a pure rebase this time — real content was added since 0b5eff9b8 (CI is now fully SUCCESS, 0 failures). Diffed the two heads directly:

CHANGELOG.md                                  |  3 +-
docs/source/changelog/1.5.0.rst               | 16 +++++++-
pcapkit/protocols/application/httpv1.py       |  9 ++++-
tests/const/test_const_enum_get.py            | 53 ++++++++++++++++++++++++---
tests/protocols/application/test_http_unit.py | 33 +++++++++++++++++

Two genuine additions, both checked:

1. _RE_STATUS is now anchored (rb'\d{3}' → rb'\d{3}\Z'). Reproduced the bug this fixes: with the old unanchored pattern, re.match(_RE_STATUS, b'2000') prefix-matches b'200', so int(para2) reads 2000, and HTTP(raw) on a status line HTTP/1.1 2000 OK\r\n... raises a bare ValueError: 2000 is not a valid StatusCode rather than the documented ProtocolError — confirmed directly (reverted just this one regex, reran). With the anchor restored, the same input raises ProtocolError: HTTP: invalid format, and the existing suite (tests/protocols/application/test_http_unit.py, 36 passed) still passes, so valid 3-digit codes are unaffected.

2. A new test hardens the "3 never-raise" registries (ProtectionAuthority, CGAType, Flags — the ones I separately verified in my original appendix). test_the_always_resolving_registries_have_nothing_to_fall_back_to now asserts they resolve with or without a default, and setUpClass adds a class cleanup (addClassCleanup(purge_modules, ['pcapkit'])) because probing ProtectionAuthority/CGAType mutates a module-global registry (extend_enum permanently registers a member). Ran tests/const/test_const_enum_get.py + tests/protocols/application/test_http_unit.py: 36 passed, 130 subtests passed, exit 0.

But this new addition introduces its own citation error, of the same species as the one already open. The new comment says *":rfc:9110#section-15 gives `status-code = 3DIGIT`, exactly three."* I checked both RFCs directly: RFC 9110 §15 ("Status Codes") is the semantics/registry section — it lists what each code *means*, and contains no ABNF at all. The `status-code = 3DIGIT` grammar rule is in **RFC 9112 §4 ("Status Line")**, line 673 and 2106 of `rfc-editor.org/rfc/rfc9112.txt` — RFC 9112 is the HTTP/1.1 message-syntax document; RFC 9110 is HTTP Semantics and doesn't carry wire-syntax ABNF. So this should cite `:rfc:`9112#section-4 (or plain 9112 §4), not 9110 §15.

❌ NEEDS CHANGES — head 33692df984bf2c936ff10d5d9718f8e01d24cd21: two RFC citation fixes needed —

1. (carried over, unchanged) RFC 959 "section 4.1" → "section 5.3" in docs/source/changelog/1.5.0.rst and tests/protocols/application/test_ftp_unit.py:71.
2. (new) the _RE_STATUS comment in pcapkit/protocols/application/httpv1.py citing :rfc:9110#section-15`` for status-code = 3DIGIT should cite RFC 9112 §4 instead.

Both are one-line text fixes; the code, tests, and CI are otherwise sound — the new _RE_STATUS anchor fix and its test are independently verified correct, and everything from my original 23:17Z appendix still stands.

[JarryShaw]

Re-pointing — PR #596, head 04aa920bf9746e95d948e18b201671e747b61618

Two things happened since my last comment (33692df98):

1. The RFC 959 citation is fixed. docs/source/changelog/1.5.0.rst and tests/protocols/application/test_ftp_unit.py:71 now both cite :rfc:959#section-5.3 and quote the actual text ("Upper and lower case alphabetic characters are to be treated identically... RETR Retr retr ReTr rETr") — exactly the passage I found. The new docstring also explicitly distinguishes this from the *other*, correct `:rfc:`959#section-4.1 citations in pcapkit.const.ftp.command (which classify command kind — access control / transfer parameter / service — a different claim entirely, and one I'd already independently confirmed as accurate in my first appendix). Good, precise fix — it didn't blanket-replace every "4.1" with "5.3", only the wrong one.

2. The head then moved again to 04aa920b, purely from a rebase past #597's merge (that PR's tcp.py/test-file bullets shifting position in the shared changelog is the only visible delta; diff between 22a31ef9→04aa920b restricted to every file this PR owns — pcapkit/const/, pcapkit/vendor/, ftp.py, httpv1.py, tests/const/, tests/protocols/application/ — is empty). No new re-verification needed for that hop.

One item from my previous comment is still open: the _RE_STATUS comment in httpv1.py still cites :rfc:9110#section-15`` for status-code = 3DIGIT, which is RFC 9112 §4's grammar, not RFC 9110's. Confirmed unchanged at this head.

❌ NEEDS CHANGES — head 04aa920bf9746e95d948e18b201671e747b61618: one remaining fix — the _RE_STATUS comment in pcapkit/protocols/application/httpv1.py should cite RFC 9112 §4, not :rfc:9110#section-15``, for status-code = 3DIGIT. Everything else (both citation and mechanism) is now verified correct.

[JarryShaw]

✅ GOOD TO MERGE — head a79c77e86. Both prior NEEDS CHANGES reasons are resolved (verified below); the coordinator's reading of the two excluded vendor templates also checks out independently. Full history: this is the fifth verdict on this PR across five heads (aab3350a9 → 0b5eff9b8 → 33692df98 → 04aa920bf → a79c77e86); every hop before this one was either a pure rebase or a citation-string fix, confirmed each time.

Re-verifying the coordinator's reading of the two excluded templates

Read both files directly rather than accepting the description.

pcapkit/vendor/pcapng/option_type.py:111-128 — the integer path is:

if key in temp_ns:
    return temp_ns[key]
return extend_enum({NAME}, '%s_unknown_%d' % (namespace, key), key, '%s_unknown' % namespace)

The constructed name embeds the key's own value (opt_unknown_40, never just 40), so it can never collide with an existing member — there is no "raw key looked up, normalised key registered" mismatch to have. The string path is if key in __members__: return getattr(...) else extend_enum({NAME}, key, default, key) — the same raw key is used for both the lookup and the registered name, with no case transformation anywhere. That is actually a second, independent reason this file can't have #582/#583's defect: the bug requires the lookup key and the registered key to differ (one raw, one normalised); here they're always identical.

pcapkit/vendor/reg/apptype.py:150-177 — same shape: the integer path's _missing_ fallback constructs 'PORT_%d_%s' % (key, proto.name) (again, unique per key), and the string path is if key in __members_proto__: return getattr(...) else extend_enum({NAME}, key, default, key) — again the same raw key both ways.

Confirmed both files are genuinely untouched: git diff fa6d18e31..HEAD -- pcapkit/vendor/pcapng/option_type.py pcapkit/vendor/reg/apptype.py pcapkit/const/pcapng/option_type.py pcapkit/const/reg/apptype.py is empty. The coordinator's reading holds — I found no reason to dispute it, and found an additional structural reason (the string-path identity) that reinforces it.

The two previously-open citation issues

RFC 959 §4.1→§5.3 (FTP case-insensitivity): fixed several heads ago, confirmed still correct at this head.

RFC 9112 vs 9110 (status-code = 3DIGIT): now fixed at pcapkit/protocols/application/httpv1.py:68-71 — cites :rfc:9112#section-4 for the grammar and correctly notes `:rfc:`9110#section-15 covers semantics/registry, not syntax. The accompanying test docstring (tests/protocols/application/test_http_unit.py) goes further and quotes RFC 9112 §4's own text ("HTTP's core status codes are defined in Section 15 of [HTTP]") as corroboration, and explicitly credits the cross-review for catching the original mis-citation. Both citations are now correct and I have nothing further to dispute here.

CI

Rollup PENDING, CheckRun tally: 2 SUCCESS, 2 SKIPPED, rest QUEUED, 0 FAILURE/CANCELLED.

All prior findings from my earlier appendices on this PR (the Command.get/Method.get mechanism, the anchored _RE_METHOD regex, the default-consulted integer path, the skip-list/never-raise reconciliation confirmed against the true base, the 9-of-11 test evidence, the pre-existing RecursionError disclosure) are unaffected by these changes and stand as previously verified.