Skip to content

fix(http): reach _guess_version's HTTP/2 arm, and resolve PayloadField names - #789

Merged
JarryShaw merged 1 commit into
mainfrom
fix/787-http-guess-version-payloadfield-case
Sep 25, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/787-http-guess-version-payloadfield-case

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

Both defects in #787, plus three more the cross-review found in the same code.

Defect 1 — chose normalising at the source (httpv1.HTTP raises ProtocolError), not suppressing ValueError in _guess_version. A bare ValueError out of that constructor is never meaningful: both read and _read_http_header already document Raises: ProtocolError, and #583's _RE_STATUS fix already treated a bare ValueError from this method as the defect. No test or caller depends on the type — beholder and _import_next_layer both catch Exception, so the Raw fallback is unaffected. Ruled out suppressing ValueError: ProtocolError is a ValueError subclass, so that widens the net to every stray stdlib ValueError and would hide real bugs as "unknown HTTP version" — and it would fix only the proxy, leaving the bare error on tcp.py's ports 80/8080, which dispatch straight to HTTPv1 and never reach _guess_version. No port binding touched.

Two further exception types were keeping the same arm dead, and are fixed here rather than left as caveats. A field line with no colon left re.split one element long, so item[1] raised IndexError — not a ValueError, so neither the new guard nor suppress(ProtocolError) caught it. An obs-fold continuation line (RFC 9112 §5.2) is exactly such a line and is legal HTTP/1 that real captures carry, so it is unfolded — the RFC's own remedy — and only a genuinely colon-less line is refused; refusing the folded form instead would have left a legal request falling through to the HTTP/2 arm, which is the mislabel this PR exists to stop. Measured on the previous head, b'GET / HTTP/1.1\r\nX-Long: a\r\n b\r\nHost: e\r\n\r\n' raised IndexError: list index out of range both directly and through the proxy, and a continuation that did carry a colon was worse still — it parsed in silence into a spurious extra field (X-Long: a plus b: c, for a folded X-Long: a b), with nothing raised at all. The unfold replaces the whole obs-fold = OWS CRLF RWS, so the accumulator is right-stripped as well as the continuation: stripping only the continuation left the OWS before the CRLF inside the value, and four of five folded/literal pairs disagreed (X: a \t over \tb gave 'a \t b' for a literal 'a b'), with a HTAB surviving where the RFC prescribes SP.

Separately, a payload under nine octets reaches httpv2.HTTP once the arm is live, and failed there with a bare struct.error — neither a ValueError, a ProtocolError, nor pcapkit's own StructError, so except ValueError, except ProtocolError and except BaseError all missed it. read converts it, and _guess_version suppresses it beside ProtocolError on the HTTP/2 arm only. The asymmetry is load-bearing: that arm is last, so nothing follows to answer in its place, whereas suppressing on the HTTP/1 arm hands the payload onward to an arm that accepts anything of ≥9 octets. An earlier revision of this PR widened both, and it was measured to be actively harmful — StructError subclasses struct.error, so with a fault injected at arm 1 a valid HTTP/1.1 request came back version='2', and over UDP/80 its protochain read UDP:HTTP/2; unknown HTTP version is only the best case, needing arm 2 to decline too. It also buys nothing: nine byte patterns × lengths 0–24 × both HTTP/1 routes gave ProtocolError 450 times out of 450, and a sweep of arm 2 found its struct.error hits are all stdlib-only and all at sizes 0–8, so no StructError.eof signal is erased there either. Precisely: of the 81 sub-9 cells (9 patterns × lengths 0–8), 76 raise struct.error and 5 parse successfully — httpv2.HTTP's own schema.length < 9 guard tests the declared length, not the buffer's, so b'\x00\x00\x0f\x04' (4 octets declaring 15) parses. The comment in _guess_version says "usually" for that reason. Zero of the 81 raise pcapkit's StructError. The residual is stated there too, and is real: a genuine httpv2 schema defect now reports as unknown HTTP version rather than crashing, which is accepted because httpv2.HTTP stays reachable directly where nothing is suppressed. The underlying defect is httpv2.HTTP's and predates this PR — HTTPv2 built directly still raises it, and HTTP(..., version=2) did too, on the base revision as much as on this one — so it is fixed at this dispatcher's boundary, where both routes into a versioned parser already normalise their failures; a length guard in _guess_version was rejected because it would have left the explicit version=2 route leaking identically and would have put RFC 7540's 9-octet frame size in the dispatcher that exists not to know it. Finally, re.split's maxsplit is passed by keyword at both sites: positionally it is a DeprecationWarning on 3.13+ and is documented to become a TypeError, a third exception type that would revive #787 verbatim. It was also firing on every HTTP parse — warnings across the test selection below drop from 8824 to 41.

Defect 2 — case-folds, and does warn. A miss here is reached only from a caller that named a protocol in source, so it is a mistake in that name, not a property of the packet, and Raw is otherwise indistinguishable from an unparsed payload. It stays a warning (RegistryWarning) rather than a raise because None → Raw is legitimate for a field with no protocol. No in-library call site passes a string, so this adds no noise. Also: the issue's own reproduction goes through __init__, which assigned _protocol directly and bypassed the setter — PayloadField(protocol='http').protocol was 'http' itself, and 'HTTP' was no better, so case was never what that path went wrong on. __init__ now assigns through the property.

_guess_version is on the real extraction path, so the remaining consequence is a protochain mislabel in pcapkit.extract() output — not an API-only curiosity. An earlier revision of this description argued the blast radius was narrow because TCP ports 80 and 8080 dispatch straight to HTTPv1. That is true but incomplete: UDP ports 80 and 8080 dispatch to this proxy, not to HTTPv1 — UDP.__proto__ maps both to pcapkit.protocols.application.http — so _guess_version runs during ordinary extraction. The affected set is derived, not enumerated. Earlier revisions of this description counted it by example and it grew 1 → 3 → 4 on successive reviews, each round finding a class the last had missed — which is a bad way to establish a blast radius. It is derivable instead:

The payloads whose proxy answer changed are exactly
{payloads httpv1 refused with a bare (non-ProtocolError) exception on base} ∩ {payloads httpv2 accepts} ∩ {payloads httpv1 still refuses},
because a bare exception is precisely what contextlib.suppress(ProtocolError) failed to catch, and that is the only way the first arm could abort rather than decline.

So the set is indexed by the bare-exception raise sites, and those are exactly the four this PR converts — verified by walking every raise-capable operation in read and _read_http_header on the base revision and attributing each by traceback frame. One row per site makes the table complete by construction:

# conversion site example payload now answers
1 read: packet.split(b'\r\n\r\n') — no separator b'not http at all' HTTP/2
2 _read_http_header: header.split(b'\r\n') — header with no CRLF HTTP/2 preface; field-less HTTP/1 request; field-less HTTP/1 response HTTP/2
3 _read_http_header: re.split(rb'\s+', startline, maxsplit=2) — start line under 3 tokens b'GET /\r\nHost: e\r\n\r\n', b'PRI\r\nHost: e\r\n\r\n' HTTP/2
4 _read_http_header: item[1] — field line with no colon b'GET / HTTP/1.1\r\nNoColonHere\r\n\r\n' HTTP/2

Site 3 is the one the enumerations kept missing: it is not "field-less" (it has a field section), not colon-less, and not fairly called "non-HTTP bytes". Note the site keys the row, but whether HTTP/2 is wrong depends on the payload — for the preface in site 2 it is the correct answer and the whole point of #787; for a field-less HTTP/1 message in the same site it is a mislabel. Over UDP/80 the mislabelling members read UDP:HTTP/2 where base read UDP:Raw.

Causes, per site: sites 2's HTTP/1 members are a pre-existing _read_http_header defect and a candidate follow-up, deliberately not fixed here because the CRLF split that refuses them is load-bearing — relaxing it makes the preface PRI * HTTP/2.0 parse as a valid HTTP/1 request (_RE_METHOD matches PRI, _RE_VERSION matches HTTP/2.0) and revives this very bug. Site 1 is inherent to a reachable second arm, since httpv2.HTTP accepts any payload of ≥9 octets with an unassigned frame type being expected traffic per RFC 9113 — any fix for #787 has it. Sites 3 and 4 are the narrow price of refusing a malformed message rather than dropping part of it.

The candidate set partitions exhaustively and disjointly into three outcomes, measured base-vs-head over a 16-payload battery: 8 → HTTP/2 (the table), 1 → HTTP/1.1 (the obs-folded request, which this PR now parses correctly — an improvement, not a mislabel), and 1 → ProtocolError (sub-9-octet payloads, which changed only in which exception they raise and are now catchable). The four classes that already raised ProtocolError on base — an unrecognised 3-token start line, a non-numeric status, a lowercase method, a bad version token — are correctly excluded: they already answered HTTP/2 before this PR and are unchanged by it.

Rather than pin that table, a test pins the invariant that bounds it: test_httpv1_never_lets_a_bare_exception_escape asserts that over a battery spanning both methods, nothing escapes httpv1.HTTP that is not a ProtocolError. A fifth bare-raising site fails there, so a sixth class cannot appear unnoticed. Against the base library that test reports 13 subtest failures.

End-to-end check. Protochains for every frame of all 23 sample captures — 1604 frames, of which 231 are HTTP — are byte-identical across the PR base, the previous revision and this one. That bounds the changes above to input classes the sample captures do not contain; note the captures reach HTTP over TCP, which dispatches straight to HTTPv1, so they do not exercise the proxy path itself.

Existing HTTP tests, before → after. Baseline at 0419c1c97 (captures generated): 59 passed. Four tests observed the old behaviour and are updated rather than silenced, each with the reason in its docstring: two asserted 'HTTP/1: invalid format', the message HTTP.read produced when it re-labelled a ValueError it no longer sees; one asserted that malformed HTTP/1 bytes leave _guess_version as a ValueError, the defect stated as the contract; and one pinned version == '2' for b'not http at all', which makes "garbage must report HTTP/2" a contract and would block a later httpv2 tightening — it now asserts that the second arm was entered, which is the property #787 is about, with the answer for real HTTP/2 bytes pinned by test_guess_version_reaches_http2_on_the_connection_preface where it belongs.

Counts, with the selection named so they can be reproduced. The 91 tests / 152 subtests quoted earlier was wrong and should not have been reported as green: it was a run in which five capture-dependent tests in test_http_runtime.py were failing for want of generated captures (verified — exactly those five fail when examples/captures/ is absent). With captures generated: 343 tests / 2117 subtests pass across tests/protocols/application/, tests/corekit/ and tests/protocols/test_construction_keyword_check_unit.py, against 335 / 2063 before this PR's revisions; the three files this PR touches are 76 tests / 194 subtests, agreeing exactly under pytest and unittest (76 = 76, checked both ways, since pytest-subtests can report a parent node as passed when only its subTests fail). http.py and httpv1.py are both at 100% line and branch, new statements and branches included. misc.py measures 72% on that selection and is unchanged by this revision — the earlier 66% → 69% was against a narrower one. mypy reports 0 findings in either changed file (95 tree-wide, unchanged). pylint under the project's own PYLINT_FLAGS — which disable design, so no R09xx appears — gives 9.47/10 against a 9.43 baseline at the first revision of this PR, with the finding set byte-identical (10 findings, all pre-existing kinds: import-outside-toplevel, protected-access, attribute-defined-outside-init, arguments-renamed, unused-argument, cyclic-import). An earlier revision of this description quoted 8.99 and a too-many-locals movement; that came from a bare pylint run using defaults rather than the project's flags, under which too-many-locals is not enabled at all. isort -l100 -ppcapkit is clean.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) bug test Pull requests that add or correct tests (test: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review at 779d4f777 (haiku, a different model from the author): NEEDS CHANGES — three items, all of which I re-measured myself.

1. The HTTP/2 arm is still dead for a whole input class, 30 lines below the fix. httpv1.py:343 does value = self.decode(item[1].strip()) after re.split(rb'\s*:\s*', field, 1), which returns a 1-element list for a colon-less field line. IndexError is not a ValueError, so neither the new guard nor suppress(ProtocolError) catches it:

obs-fold continuation (RFC 9112 5.2)   httpv1 direct  -> builtins.IndexError: list index out of range
obs-fold continuation (RFC 9112 5.2)   proxy guess    -> builtins.IndexError: list index out of range
field line with no colon               proxy guess    -> builtins.IndexError: list index out of range

Pre-existing, but the obs-fold form is real wire input, and the PR's own test docstring claims it covers "both of the unpackings that a non-HTTP/1 payload lands short on". There are three, and the third is not a ValueError.

2. A regression the PR introduces: stdlib struct.error now escapes where ValueError used to. Measured both trees on the proxy:

BASE   0,1,4,8,9,15 octets -> builtins.ValueError  isValueError=True   (all six)
PR     0,1,4,8    octets   -> struct.error         isValueError=False
PR     9,15       octets   -> parsed version=2

It is the bare stdlib struct.error, not pcapkit's StructError, so except ValueError, except ProtocolError and except BaseError all miss it. On base these inputs were all catchable. One line fixes it.

3. The blast-radius reasoning names TCP and omits UDP — where it actually lands.

TCP.__proto__ HTTP ports: {80: httpv1, 8080: httpv1}
UDP.__proto__ HTTP ports: {80: http (the PROXY), 8080: http}

So _guess_version is on the extraction path, and the two consequences the PR calls unavoidable are protochain mislabels in pcapkit.extract() output — a valid field-less HTTP/1.0 now reads UDP:HTTP/2 where base read UDP:Raw. Worth saying plainly in the body rather than leaving a reviewer to infer it is API-only.

The core fix is right and belongs in httpv1 — the review confirmed the arm is reachable for real preface bytes, the Raw fallback is intact, __cause__ chaining survives, PayloadField resolves on both paths for both cases, coverage 99→100% and 66→69%, and mypy unchanged at 4 pre-existing errors. It also derived rather than repeated the author's warning: _RE_METHOD matches PRI and _RE_VERSION matches HTTP/2.0, so a tolerant split really would re-kill the arm.

Two smaller items to fold in: httpv1.py:310/:314 pass maxsplit positionally, which is a DeprecationWarning today and a TypeError later — also not a ValueError, so #787 would recur; and the PR body says 91 tests where the commit message says 96, the 91 being a run with five capture-dependent failures. Routing all of it.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw
JarryShaw force-pushed the fix/787-http-guess-version-payloadfield-case branch from 779d4f7 to b9cf62f Compare September 25, 2026 15:54
@JarryShaw

Copy link
Copy Markdown
Owner Author

Revised to b9cf62f9d. All three findings held, and the worker corrected my own framing of the second one — it was right. Measured on base 0419c1c97 myself:

proxy _guess_version, b'x'*n and b'\x00'*n   4,8,9,15 octets -> builtins.ValueError  (isValueError=True)
HTTP(..., version=2)                         4,8    octets    -> struct.error         (isValueError=False)
HTTPv2 direct                                4,8    octets    -> struct.error         (isValueError=False)

So the bare struct.error pre-exists in httpv2's schema — FieldBase.length → struct.calcsize on a template built from a negative length. What this PR introduced is a new route to it, via the now-reachable arm. I called it "a regression the PR introduces"; that overstates it. It still broke the proxy's documented Raises: ProtocolError contract, so it still had to be fixed — but the cause is not this PR's.

Its two design choices, both justified by measurement rather than preference:

  • Obs-fold: unfold, not refuse. A folded continuation containing a colon mis-parsed silently — X-Long: a plus a bogus b: c instead of X-Long: a b: c. Refusing cannot fix a silent misparse without first detecting the continuation, and having detected it, unfolding is RFC 9112 §5.2's own remedy and one line more. Refusing would also have dropped a legal HTTP/1.1 request into the HTTP/2 arm — manufacturing a fresh instance of the mislabel this PR exists to stop.
  • Convert at the dispatcher boundary, not a length < 9 guard. It measured that a guard in _guess_version leaves HTTP(..., version=2) leaking identically — a route my brief did not cover — and it would hard-code RFC 7540's frame size into the module whose job is not to know version internals. It probed ≥9-octet inputs to confirm a guard would suffice, then rejected it on honesty rather than adequacy.

Finding 4 had a side effect worth having: the positional-maxsplit warning fired on every HTTP parse. Warnings across the selection dropped 8824 → 41.

Finding 5: both numbers were wrong, mine included. Neither 91 nor 96 reproduces against any selection — that directory collects 95, the touched files 68. Replaced with figures that name their selection: 340 tests / 2080 subtests across the three mandated paths (was 335/2063), 73 / 157 for the touched files, agreeing exactly under pytest and unittest. My diagnosis of why 91 was wrong is confirmed exactly: with examples/captures/ absent that directory gives 5 failed / 90 passed, all five in test_http_runtime.py.

It also merged current main in a scratch worktree and re-ran, since GraphQL reported MERGEABLE/BEHIND — 73 passed there, plus #786's isort gate and tier guard at 103 passed, which specifically clears its new import struct.

Label back to review: pending; a re-review on a different model is going out before I set a verdict.

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Sep 25, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Delta re-check at b9cf62f9d (haiku, resumed): GOOD TO GO on all three blockers, and it withdrew its own framing of finding 2 — re-deriving on base itself and conceding that the struct.error pre-exists in httpv2 on both the explicit and direct routes, so the PR opened a route rather than introducing the defect.

The strongest evidence it produced was unprompted: full pcapkit.extract() over all 23 generated captures on all three trees, protochains serialised and diffed — base 0419c1c97 vs new b9cf62f9d: IDENTICAL, old 779d4f777 vs new: IDENTICAL, with 231 real HTTP/1 frames (225 IPv4 + 6 IPv6) through the rewritten _read_http_header. It also checked the accumulator for quadratic blow-up: 20,000 continuation lines / 80,033 header bytes parses in 12 ms, flat. No DoS vector.

Counts reproduced exactly: 340/2080 vs 335/2063, 73/157 agreeing under pytest and unittest, http.py and httpv1.py at 100% line and branch (branch 28→36, all covered), misc.py 72%, mypy 174 lines identical, pylint histogram identical. Warnings 8824 → 41 confirmed.

Three items it raised as non-blocking, and I am routing all three — two are real rather than cosmetic:

  1. rstrip() the unfold accumulator. fields[-1] += b' ' + line.strip() strips the continuation but not the accumulator, so OWS before the CRLF survives. With a trailing HTAB the result is non-conformant: RFC 9112 §5.2 defines obs-fold = OWS CRLF RWS and prescribes replacing the whole thing with one or more SP, and a HTAB survives inside the value. Measured: folded=[('X','a\t b')] vs literal=[('X','a b')]. One character fixes it and makes the docstring's agreement claim true in general rather than for the one input the test uses.
  2. Narrow arm 1's suppression back to ProtocolError. Swept 9 byte patterns × lengths 0-24 × 2 routes: 225/225 and 225/225 ProtocolError, zero struct.error-family hits on the HTTP/1 route. So the widening buys arm 1 nothing, and it costs: issubclass(StructError, struct.error) is True, so injecting a StructError into arm 1 on well-formed HTTP/1.1 bytes yields protochain=UDP:HTTP/2 — a confident mislabel of valid HTTP/1, which is the failure this PR exists to stop. suppress(ProtocolError) on arm 1 and suppress(ProtocolError, struct.error) on arm 2 keeps the whole fix.
  3. The body says "One consequence survives" where three input classes now read UDP:HTTP/2 instead of UDP:Raw. The previous revision was more complete here.

Unprompted find, out of scope for #787 but a real bug: six sites in pcapkit/vendor/ pass a flag as re.sub's positional count — re.sub(r'\r*\n', ' ', tmp1, re.MULTILINE) at reg/ethertype.py:95, plus tcp/mp_tcp_option.py:65, tcp/option.py:78, tcp/flags.py:139, ipv6/option.py:89. re.MULTILINE is 8, so these mean "replace at most 8 occurrences". Filing separately.

Holding review: pending until the three land.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Revised to 714b864ea. All three items applied, and two of my own numbers were undercounts — the worker measured more than I relayed.

Item 1, verified myself on the new head:

no trailing OWS  agree=True  HTAB_survives=False  [('X', 'a b'), ('Host', 'e')]
trailing SP      agree=True  HTAB_survives=False  [('X', 'a b'), ('Host', 'e')]
trailing HTAB    agree=True  HTAB_survives=False  [('X', 'a b'), ('Host', 'e')]
mixed OWS        agree=True  HTAB_survives=False  [('X', 'a b'), ('Host', 'e')]
disagreeing pairs: 0 of 4

I relayed the reviewer's two disagreeing pairs; pre-fix it was 4 of 5, with HTAB surviving in two. The new test asserts folded-vs-literal equivalence over a table rather than against hand-written values — which is the property RFC 9112 §5.2's remedy is actually defined by, and a better test than the one I asked for.

Item 2, verified myself — a fault in arm 1 now propagates instead of becoming HTTP/2:

issubclass(StructError, struct.error) = True
stdlib struct.error    -> propagates struct.error                              (correct)
pcapkit StructError    -> propagates pcapkit.utilities.exceptions.StructError  (correct)
StructError(eof=True)  -> propagates pcapkit.utilities.exceptions.StructError  (correct)

0/4/8 octets -> ProtocolError: unknown HTTP version   9/15 octets -> version=2

So the narrowing removed the mislabel without unfixing finding 2. The worker also rewrote the comment around the real principle — an arm that is not last hands the payload onward when it swallows, so only the last arm may widen — which is a better rule than "narrow arm 1".

It checked arm 2 unprompted, since my StructError.eof argument applied there too: 9 patterns × lengths 0-40 through HTTPv2 direct gave 76 stdlib-only struct.error hits, zero pcapkit StructError, all confined to the sub-9 class. So the prescribed shape erases no eof signal and needs no three-branch try.

Item 3: four classes, not three. My count conflated the field-less request and response. Table in the body now, numeral dropped, each cause attributed.

And it volunteered a bound on its own strongest evidence, which I want on the record: the 23-capture protochain diff is IDENTICAL over 1604 frames (base vs narrowed, and widened vs narrowed), with 231 HTTP frames — but every HTTP frame in the corpus arrives over TCP, which dispatches straight to HTTPv1 and never reaches _guess_version. So that result proves the httpv1 changes inert on real traffic; it does not exercise the proxy path, which has no sample capture. That limit is worth more than the clean diff.

Counts: 342/2093 on the mandated selection, 75/170 on the touched files agreeing exactly under pytest and unittest, both changed modules at 100% line and branch, mypy 0 in either file, pylint 8.99 with no new finding.

Re-review dispatched on a different model; holding review: pending until it lands.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Delta re-check at 714b864ea (haiku, resumed): GOOD TO GO. It found no defect, confirmed both items I said were bigger than either of us had claimed, and settled the transport bound by instrumentation rather than inference — which is the part I most wanted.

total frames in corpus       : 1604
HTTP frames by transport     : {'TCP': 231}          <- zero over UDP
UDP frames on port 80 or 8080: 0   (34 distinct UDP port pairs, all enumerated)
_guess_version entered       : 0 time(s) across all 1604 frames

So the IDENTICAL protochain diff proves the httpv1 changes inert on 231 real HTTP/1 frames and says nothing about the proxy path, exactly as the worker's own caveat stated. All proxy-path evidence rests on unit tests and synthetic probes, and the body says so at line 44.

A false positive it fell into and corrected, worth recording. Its first instrumentation patched the proxy's __init__ and counted 252 constructions, reading as "the proxy is exercised". It is not: issubclass(httpv1.HTTP, proxy HTTP) is True, httpv1.py:36 does from …http import HTTP as HTTPBase, and neither subclass defines __init__, so the probe was counting versioned classes through inheritance. Anyone re-checking this bound that way will get the same wrong answer.

It also caught that its own first pass used most_common(20) where there are 34 distinct UDP port pairs — a truncated view could have hidden an 80/8080 pair. It re-ran exhaustively.

Two corrections to numbers I relayed:

  • The arm-2 sweep is 81, not 76. And 81 is forced: 9 patterns × 9 lengths (0–8) = 81, corroborated by the other buckets summing to 288 = 9 × lengths 9–40. The load-bearing conclusion is unchanged — zero pcapkit StructError, zero eof=True, every hit sub-9 — so the two-branch shape is sufficient.
  • "pylint 8.99 with only the pre-existing too-many-locals" was wrong twice. Under the project's own make pylint flags design is disabled, so no R09xx appears at all and the score is 9.51; with design enabled it is 9.15 and too-many-locals is not alone (R0917, R0912, R0401 and others appear). The 8.99 was pylint's cached previous-run score echoed in its footer, not that run's result. The substantive claim — no new finding versus the previous head — holds, by identical histogram.

Both mutation checks bit exactly: reverting the rstrip gave FAILED (failures=6), decomposing precisely to my "4 of 5 pairs, HTAB surviving in two"; re-widening arm 1 gave FAILED (failures=3), one per fault flavour. It also fuzzed 20,000 payloads through the guess path — 0 escaped as anything other than ProtocolError, so the new Raises: docstring holds.

Two items I am routing rather than shipping — details on the thread.

…d names

- httpv1.HTTP raised a bare exception for any payload that is not an HTTP/1
  message, at four sites: the header/body split in read, the header's CRLF split
  and the start-line split in _read_http_header, and item[1] on a field line
  with no colon. HTTP._guess_version falls through on ProtocolError alone, so
  the HTTP/1 attempt aborted the guess rather than failing it and the HTTP/2 arm
  below was dead code -- for every one of those four input classes, not just the
  first. All four now raise ProtocolError, which is what each method's Raises:
  section already documented; the three unpacking sites chain their cause.
- Those four sites are the whole of it, and a test pins the invariant rather
  than the list: over a battery spanning both methods, nothing escapes
  httpv1.HTTP that is not a ProtocolError. A fifth bare-raising site would fail
  there, which is what keeps the set of affected payloads derivable.
- An obs-fold continuation line (RFC 9112 5.2) is legal HTTP/1 and is now
  unfolded, the RFC's own remedy, rather than read as a field of its own: with
  no colon it raised that IndexError, and with one it parsed in silence into a
  spurious extra field. The production is OWS CRLF RWS and the whole of it is
  replaced by one space, so the accumulator is right-stripped too -- otherwise
  the OWS before the CRLF survived into the value and a folded message
  disagreed with its literal equivalent, HTAB included.
- A payload under nine octets reached httpv2.HTTP for the first time once the
  arm was reachable, and usually fails inside the schema with a bare
  struct.error -- neither a ValueError nor a ProtocolError nor pcapkit's
  StructError, so no caller could catch it. read converts it, and _guess_version
  suppresses it on the last arm only: an arm that is not last hands the payload
  onward when it swallows, so widening the HTTP/1 arm turned an injected fault on
  valid HTTP/1.1 bytes into a confident version='2'. Nothing reaches a
  struct.error through httpv1 anyway, 450 payload/route pairs tried. The
  residual is stated in the comment: a real httpv2 schema defect now reports as
  unknown HTTP version rather than crashing. That defect is httpv2.HTTP's and
  predates this change -- HTTPv2 built directly still raises it, and is left for
  its own review.
- re.split's maxsplit is passed by keyword at both sites: positionally it is a
  DeprecationWarning on 3.13+ and is documented to become a TypeError, which
  would revive #787 from a third exception type.
- PayloadField.protocol looked a str up in __proto__ as given, while the registry
  is keyed on the upper-cased class name, so a name not already upper-case
  resolved to None and silently yielded Raw. It now folds case, and warns with
  RegistryWarning for a name that is genuinely unregistered.
- PayloadField.__init__ wrote _protocol directly, bypassing that setter, so a
  name given to the constructor was stored verbatim and handed back as a str. It
  now assigns through the property.
- Four existing tests pinned the old behaviour and are updated rather than
  silenced: two asserted 'HTTP/1: invalid format', the message HTTP.read produced
  when it re-labelled a ValueError it no longer sees; one asserted that malformed
  HTTP/1 bytes leave _guess_version as a ValueError; and one pinned the answer
  the reachable arm gave for garbage rather than that the arm was reached, which
  would have made "garbage reports HTTP/2" a contract.

Refs #787. 343 tests and 2117 subtests green across tests/protocols/application/,
tests/corekit/ and tests/protocols/test_construction_keyword_check_unit.py with
captures generated; the three files this touches are 76 tests and 194 subtests,
agreeing exactly under pytest and unittest. http.py and httpv1.py at 100% line
and branch. Protochains over all 23 sample captures, 1604 frames, are byte
identical to the base revision's.
@JarryShaw
JarryShaw force-pushed the fix/787-http-guess-version-payloadfield-case branch from 714b864 to 11bb769 Compare September 25, 2026 17:52
@JarryShaw

Copy link
Copy Markdown
Owner Author

Revised to 11bb7693a. And it refuted one of my own corrections — I was wrong and it was right.

I told it the arm-2 sweep was 81, not 76, and that "81 is forced: 9 patterns × 9 lengths (0–8)". That arithmetic assumed every sub-9 cell raises struct.error. Five do not. Verified on base myself:

4 octets declaring 15 -> PARSED (no struct.error)
settings pattern, lengths 4-8 -> parsed, parsed, parsed, parsed, parsed

So 81 − 5 = 76, its original number. And the reason is a real sub-finding worth having: httpv2.read's guard is schema.length < 9 on the declared length, not the buffer's, so b'\x00\x00\x0f\x04' — four octets declaring fifteen — parses as HTTP/2. It also softened the code comment's absolute "fails inside the schema machinery" to "usually fails", since the outcome is buffer-dependent. My conclusion held (zero pcapkit StructError, zero eof=True, all struct.error sub-9), but the count I published was wrong.

It also corrected my diagnosis of the pylint number. I said the 8.99 was pylint's cached previous-run score. Wrong — the footer reads rated at <this run> (previous run: <cached>), so the leading number is current; 8.99 was that run's real score under bare defaults. The substantive point stands — it should have used PYLINT_FLAGS, which disables design so R09xx never fires — and measured properly it is 9.47 vs 9.43 baseline with identical finding sets. It reports 9.47 where I relayed 9.51, likely a different file scope; I am not claiming either as verified.

Item 1 landed as a derivation, and the answer is 4 rows, not 5. It enumerated every raise-capable operation in base read/_read_http_header — nine candidates, exactly four that can raise a non-ProtocolError — and verified the partition by attributing each payload by traceback frame:

DERIVATION HOLDS: True
candidates (bare exception on base): 10
  -> now HTTP/2: 8   -> now HTTP/1.x: 1 (obs-folded, correctly)   -> now ProtocolError: 1
partition exhaustive and disjoint: True True

Two things the site-keyed view exposed that the example-keyed one hid: my "field-less request" and "field-less response" rows were the same site, and that site's members include the HTTP/2 preface where HTTP/2 is the correct answer. So the row is keyed by site and the body states per row whether the answer is wrong.

And a test now bounds it instead of a list. test_httpv1_never_lets_a_bare_exception_escape asserts nothing escapes httpv1 that is not a ProtocolError, over a 20-payload battery across both methods — 13 subtest failures against the base library, so it bites. As the worker put it, that test is what catches a fifth site rather than the next reviewer.

Both routed comment items are in, including the arm-3 clause. Counts: 343/2117 on the mandated selection, 76/194 on the touched files agreeing under both runners, both changed modules 100% line and branch, protochain still IDENTICAL over 1604 frames.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Delta re-check at 11bb7693a (haiku, resumed): GOOD TO GO. The derivation holds under independent attack.

It re-enumerated the sites itself before reading the author's list and landed on the same four, then attributed every payload by traceback frame on base. Two things that settles:

  • My two "field-less" rows really were one site (:288), and that site's members include the HTTP/2 preface where HTTP/2 is the correct answer. Site :320 shows it again — the legal obs-folded request shares it with the colon-less line, and there HTTP/1.1 is correct. So a row's wrongness is a property of the payload, not the site, and its own "fifth class" was site :289, genuinely distinct. In its words: a four-site partition says both things at once; a five-row table said neither.
  • Completeness checked two ways, no fifth site. It examined self.decode (6000 random bytestrings → 0 escapes; chardet named 68 encodings and codecs.lookup resolved every one) and _read_http_body (body is return body). Then fuzzed 20,000 payloads through base httpv1: exactly three exception kinds, no fourth — 16796 ValueError, 1999 ProtocolError, 361 IndexError, 844 parsed.

And it exhaustively verified the int(para2) exclusion — all 16,777,216 three-byte sequences: exactly 1000 match _RE_STATUS, and int() rejects none. Arabic-Indic ٢٠٠ and fullwidth 200 don't match, because \d is ASCII-only in a bytes pattern, and \Z rejects the trailing newline that $ would allow. #583's fix still holding.

The 76-vs-81 question is settled, and both numbers were right about different things. Its nine patterns × nine sub-9 lengths = 81 cells that all raise — of which 76 are struct.error and the rest ProtocolError — because none of its nine patterns declares ≥9 when truncated. My five parsing cells used a frame-shaped pattern declaring 15. So it is a pattern-set difference, not arithmetic; what was actually wrong was the inference "81 is forced by 9 × 9", which it concedes. The shared conclusion is untouched: zero pcapkit StructError, zero eof, two-branch shape sufficient.

pylint settled too: 9.47 vs 9.51 is purely file scope — 9.47 for the two changed modules, 9.51 with misc.py added.

One thing it flagged as derived rather than observed, which I want on the record: it did not re-run the 1604-frame protochain extraction, because the source delta has no non-comment line and httpv1.py is byte-identical to 714b864ea where it measured IDENTICAL. So that result follows by construction, not by measurement, and it said so rather than re-asserting it.

Counts: 343/2117 mandated, 76/194 touched files agreeing under both runners, http.py and httpv1.py both 100% line and branch, mypy 0 in either changed file.

Setting review: good-to-go. Unpublished and unmerged — yours to take. Its highest-value finding is filed separately, below.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 25, 2026
@JarryShaw
JarryShaw merged commit 0928abb into main Sep 25, 2026
31 checks passed
@JarryShaw
JarryShaw deleted the fix/787-http-guess-version-payloadfield-case branch September 25, 2026 18:34
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 25, 2026
JarryShaw added a commit that referenced this pull request Sep 25, 2026
…k included

`main` moved from 73f09ae to 4530424 while this PR sat open, and the 1.5.0
section cited none of the 25 commits in between. Ten new bullets cover thirteen
of them, appended in merge order, with the file's own `**a breaking change**`
lead sentence on the three that are breaking:

- #754 -- AppType split into per-transport registries; the 1,004 portless and
  704 transportless rows stop being members. Breaking.
- #764 -- an out-of-range port in `AppType.get` is refused, not minted, so
  `TCP.make(srcport=99999)` raises; per-transport `_missing_` spans. Breaking.
- #778 -- `@final` enforced at runtime on `Info`/`Schema`: bare `@final` raises
  `InfoError`/`SchemaError` at first construction, deriving from a finalised
  class raises, and `SchemaError` is a `ValueError` where a caller may have
  been catching `TypeError`. Breaking.
- #772 (with #790's docstring reword), #766, #759, #787, #794 (with #791's
  citation repoint), #792/#798 and #802 -- the remaining seven.

Also re-ran the citation sweep against `origin/main` rather than the checkout.
One stale line number fixed: the `httpv2.py` `header.length != 9` guard the
`#692` entry calls out moved from `:562` to `:650` under #789 and #802. The
preamble's "reaching #726" becomes #805, the new maximum reference. Verified
unmoved on 4530424: `protocol.py:1411`, `schema/internet/ipv4.py:336`,
`traceflow.py` 146/149/162/424, the four `:type:` fields in `engine.rst`,
`reassembly.rst` and `traceflow.rst`, and `EXPECTED_FAILURES` at 43 entries.

Carries the previous round's #651/#646 corrections unchanged. Two literals were
reflowed so no ``literal`` wraps a line, which the generator's residual guard
refuses. `changelog_md.py --check` exit 0; `test_changelog_md.py` 47 passed,
37 subtests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 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.

1 participant