fix(httpv2): restore END_STREAM on rebuild, and seed the parsed flag accumulator as Flags (#652, #650) - #669
Conversation
JarryShaw
added
fix
#669 One bullet, because it is one round trip with a defect on each side of it, in the same two files: `_make_http_data` never read `frame.flags` on the construct side, and `FrameType.post_process` seeded its accumulator with a bare `0` on the parse side. The bullet leads with what changes rather than with the mechanism, since both halves alter output: the reconstructed DATA frame's flags octet, and the dumped `__value__` of a flagless frame. It says why #650 was worth fixing at all, which its issue had left as an open question -- the dump rendered `__value__` as a JSON number for a flagless frame and a JSON string for every other frame in the same capture, so the fix removes a type inconsistency rather than introducing one. It also records two things a reader would otherwise be surprised by. The seed is guarded rather than unconditional, because `FrameType.Flags` has no members and a memberless `enum.Flag` subclass refuses `Flags(0)` -- the one-token fix the issue proposed would have crashed six of the twelve frame schemas. And a DATA round trip is still lossy after this, for the unrelated mis-parenthesised length callbacks filed as #668, so the entry does not let the reader infer a clean round trip that does not exist yet. The `TypeError` message had to sit on one line: `util/changelog_md.py` rejects a `` literal spanning a line break with `ResidualMarkupError`, since its six conversion rules do not cover it. 37 lines added to the entry file; `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited. `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests. Committed from a detached HEAD on 367b6e6 and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree and could not be taken here. Note 367b6e6, not the 69a6e13 I was given: the branch had already moved on with #665's, #651's and #661's entries. Refs #652 Refs #650
f823c57 to
831c783
Compare
…entry The bullet added in d14577d said a memberless `enum.Flag` subclass "refuses `Flags(0)` outright", flatly. That is only true from Python 3.12, where the enum rewrite made `EnumType.__call__` raise for an enum with no members; earlier interpreters take the plain value-lookup path and hand back a pseudo-member. Measured on the two available here: version 3.14.7 members: 0 Flags(0) -> TypeError: <flag 'Flags'> has no members version 3.7.16 members: 0 Flags(0) -> OK <Flags.0: 0> `requires-python` is `>=3.6`, so the unqualified form overstated it. Three words added, no other change to the bullet: the guard in #669 is correct on every supported interpreter either way, being keyed on the memberless-ness rather than on the refusal. The same imprecision was corrected in #669's own source comment and PR body, and its test now gates only the `TypeError` assertion behind `sys.version_info >= (3, 12)` -- CI runs the unit tier on 3.10 through 3.15, so an unconditional `assertRaises` would have gone red on the older two. `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited; `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests. Committed from a detached HEAD on 6a956c4 and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree. The branch had moved on with #648's and #649's entries since d14577d. Refs #652 Refs #650
…Flags - `_make_http_data` was the only one of the six `_make_http_*` methods that never read `frame.flags`, so a DATA frame parsed with END_STREAM set rebuilt with the bit clear and the flags octet went 0x01 -> 0x00. It now restores `frame.flags.END_STREAM` the way its five siblings restore theirs. PADDED was unaffected, being re-derived from `pad_len`. - `FrameType.post_process` seeded its flag accumulator with a bare `0`, which `|=` promotes only as a side effect, so a flags octet of 0x00 -- routine in HTTP/2 -- left `__flags__` a plain `int` against a declared `Flags`, and a membership test against it raised `TypeError`. It now seeds `self.Flags(0)`, matching the construct path's six existing sites. The seed is guarded because a memberless `enum.Flag` subclass refuses `Flags(0)`, which the five frame schemas inheriting `FrameType.Flags` unchanged would otherwise hit. - Extended the `_make_http_data` frame stub, which had no `flags` attribute at all, and pinned the accumulator's type where the old assertion was type-blind. Both alter output, so this is labelled breaking: the rebuilt flags octet, and a flagless frame's dumped `__value__` going from `0` to `"Flags::None [0]"`. tests/protocols/application/: 67 passed, 39 subtests, exit 0. Fixes #652 Fixes #650
831c783 to
1b9388d
Compare
…11, not 3.12 b2ec64b qualified the claim with the wrong version. The refusal does not start with a 3.12 change -- it starts at 3.11, and 3.12 only reworded the message. The cross-review on #669 caught it; I had inferred 3.12 from the message text I happened to measure on, which is the wrong evidence for a boundary. Measured across every interpreter available here rather than inferred, with a bare `class Flags(enum.IntFlag): pass`: 3.8.20 Flags(0) -> OK <Flags.0: 0> 3.9.25 Flags(0) -> OK <Flags.0: 0> 3.10.21 Flags(0) -> OK <Flags.0: 0> 3.11.15 Flags(0) -> TypeError: <flag 'Flags'> has no members defined 3.12.13 Flags(0) -> TypeError: ... has no members; specify `names=()` ... 3.14.7 Flags(0) -> TypeError: ... has no members; specify `names=()` ... Confirmed in CPython's source, not just behaviourally. 3.11's `enum.py:1117`, inside `Enum.__new__`, raises `TypeError("%r has no members defined" % cls)` when `not cls._member_map_`, and it runs *before* the `_missing_` hook that manufactured the pseudo-member on 3.10. 3.10's `enum.py` has no such raise -- its only "has no members" occurrence is a comment at :616. 3.11 is also where the metaclass was renamed (`class EnumType(type)` at :479 with `EnumMeta = EnumType` at :1052, against 3.10's `class EnumMeta(type)` at :161), so "the enum rewrite" is the 3.11 release. One word in the bullet. #669 carries the matching correction to its source comment and to its test's `sys.version_info` gate, which had been skipping the assertion on 3.11 -- a version the unit-test matrix runs -- even though 3.11 does raise. `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited; `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests. Committed from a detached HEAD on b2ec64b and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree. Refs #652 Refs #650
Cross-review: GOOD TO GO (after one round of NEEDS CHANGES, now fixed)Independent cross-review on a different model (Sonnet), briefed to falsify rather than bless — a verdict per load-bearing claim, on evidence the reviewer obtained itself rather than from this PR's transcript. Read-only throughout; it never touched the branch. No model substitution was needed. It ran in two rounds, and the second round found a real error, so the honest headline is: GOOD TO GO on the first pass's ten claims, NEEDS CHANGES on the amendment, and the change it asked for has landed. Round 1 — GOOD TO GO on all ten claims
It derived #652 from its own It built its own dumper instances for claim 3 and reached On the guard-inconsistency question it traced every use site and reported that the six frame types declaring Round 2 — NEEDS CHANGES: the version boundary was wrongRound 1 ran against and confirmed in CPython's source rather than only behaviourally: 3.11's Consequence of the error, stated plainly: not a red build, since under-gating only skips an assertion that would have passed. But on 3.11 — a version the unit-test matrix runs — the test silently failed to exercise the thing its own comment existed to pin, and the source comment shipped a false statement about CPython. Both are corrected at the head ( The reviewer independently re-ran the before/after on the amended head and reproduced it, and confirmed both tiers green there. The
|
1 participant
Two adjacent flag defects in the HTTP/2 tier, filed separately and fixed together because they share the two files.
Fixes #652—_make_http_datawas the only one of the six_make_http_*methods that never readframe.flags, soEND_STREAMwas silently dropped on a parse → reconstruct round trip.Fixes #650—FrameType.post_processseeded its flag accumulator with a bare0, leaving__flags__a plainintfor a flagless frame where both the schema and the data model declare aFlags. #650 was filed as an open question, not an asserted defect, on the grounds that no consumer observed the difference. That turned out to be false, and the measurement below is what settles it.Measured on
0c7f2b7c9(the merge base), CPython 3.14.7, tree asserted against the worktree rather than the editable install:#652 — before and after
Parsing a DATA frame and feeding the resulting data object straight back to
_make_http_data:And through
make, on the constructed header's flags octet — i.e. what a round trip writes to the wire:All five siblings restore theirs on both sides, which is the specification this follows rather than a mechanism invented here:
PADDEDwas never affected, being re-derived frompad_lenrather than read fromframe.flags.#650 — the dump output does change, so it is not unobservable
#650's "not established" section says no consumer observes the plain
int, having found dump output identical on both paths. The cross-review on #634 had found the equivalent TCP fix did change dump output, from int0to the string"Flags::None [0]". The same thing happens here. Real parsed frames, rendered through this library's ownmake_dumper:So the observable defect is a type inconsistency in the dump, not merely an internal type:
__value__was a JSON number for a flagless frame and a JSON string for every other frame, within the same capture. It is consistently a string now. That is the same argument #634 made for TCP'sconnection, and #634 shipped withfix+breaking.The
TypeErrorgoes away too — #616's message verbatim:Flags(0) == 0isTrue, so every int-safe consumer the issue enumerated is unaffected.The literal
Nonein"Flags::None [0]"is the pre-existingpcapkit/dumpkit/common.py:216defect that #634 already identified and left to its own change;Flags(0).name is Nonefor a bitless pseudo-member. Not touched here — that file belongs to another change in flight.The one-token fix in the issue would have crashed
#650 proposes "the fix is one token —
flags = self.Flags(0)". On Python 3.11 and later that breaks parsing outright for six of the twelve frame schemas.FrameType.Flagsdeclares no members, and from the 3.11 enum rewrite a memberlessenum.Flagsubclass refuses instantiation (measured on 3.14.7):So the seed is guarded —
self.Flags(0) if self.Flags.__members__ else 0. The five inheriting schemas keep the plainint, which nothing observes because all five passflags=Noneinto their data objects. The test pins both the guard and the reason for it, so it cannot be "simplified" back into a crash.That version dependence is real, and locating it took two tries. CI runs the unit tier on 3.10 through 3.15 (
python -m pytest -q --ignore=tests/integration), so an unconditionalassertRaises(TypeError)would fail on any interpreter that does not refuse. The boundary is 3.11, measured across every interpreter available rather than inferred from a message string:Confirmed in CPython's source too. 3.11's
enum.py:1117, insideEnum.__new__, raisesTypeError("%r has no members defined" % cls)whennot cls._member_map_, and it runs before the_missing_hook that manufactured the pseudo-member on 3.10; 3.10'senum.pyhas no such raise, its only "has no members" occurrence being a comment at:616. 3.11 is also where the metaclass was renamed —class EnumType(type)at:479withEnumMeta = EnumTypeat:1052, against 3.10'sclass EnumMeta(type)at:161— so "the enum rewrite" is the 3.11 release and 3.12 only reworded the message.A first pass of this PR gated on
(3, 12)and was wrong, having inferred the version from the message text measured on 3.14 rather than from the boundary itself. The cross-review caught it; it is corrected here, in the gate and in both comments. The effect of the error was under-assertion rather than a red build — on 3.11 the block was skipped though 3.11 does raise — but it was also a false statement in a shipped source comment.The test asserts the memberless-ness unconditionally, that being the predicate the guard actually keys on, and gates only the
TypeErrorbehindsys.version_info >= (3, 11). The guard in the source is correct on every supported interpreter either way: necessary from 3.11, harmless before.These are stdlib
enum, notaenum— verified, not assumedpcapkit/const/tcp/flags.pywas read, not edited — it isaenum, it declares no_missing_, and #661 owns const changes.Failing, then passing
Both sides run from immutable
git archivesnapshots of0c7f2b7c9, with each file hashed. The test file is byte-identical across the two runs (6bfb2b2f…, the head's own); only the two source files differ.Before —
schema/…/httpv2.py6a155ba5…,application/httpv2.py9e50fe92…:with, at the #652 subtest:
Each new test's other subtest passes on that side (
end_stream=False,flags='END_STREAM'), so both fail for the reason claimed rather than incidentally.After —
schema/…/httpv2.py745c5aaa…,application/httpv2.py3a404359…:Exit codes read from a file, not a pipeline.
Coverage does not go backwards
The
end_streamsite and theflags = 0seed both already executed, so fixing them moves no coverage number; the subtest count is the real evidence.coverage run --source=pcapkitovertests/protocols/application/:0c7f2b7c9protocols/application/httpv2.pyprotocols/schema/application/httpv2.pyprotocols/data/application/httpv2.pyAll three modules stay at 100%. The schema statement count is unchanged because a conditional expression is still one statement; both arms are exercised (
DataFrameandUnassignedFrame).Full tiers, in the worktree:
tests/protocols/application/67 passed, 39 subtests, exit 0;tests/foundation/registry/test_protocols.py+tests/test_docstring_contract.py11 passed, 82 subtests, exit 0.EXPECTED_FAILURES— nothing movedtests/protocols/test_option_roundtrip_unit.pystill passes: 6 passed, 358 subtests, exit 0, matching #634's baseline exactly.EXPECTED_FAILURESwas imported, not grepped (**unpacking hides it) — 45 entries, of which exactly one is HTTP/2:Unrelated to flags, still failing in the declared way, not removed. The test passing means every entry still fails as declared and nothing else regressed, so no entry became stale.
Found and deliberately NOT fixed — filed as #668
While measuring #652's round trip the payload came back empty, which turned out to be a third and larger defect in the same schema file: three payload length callbacks are mis-parenthesised, so an unpadded DATA, HEADERS or PUSH_PROMISE frame parses with its entire payload silently dropped.
pcapkit/protocols/schema/application/httpv2.py:195,:237,:329all readA conditional expression binds looser than
-— confirmed against the AST, the top-level node is theIfExpwithorelse=Constant(value=0)— so the unpadded arm asks for zero octets instead of subtracting zero. The intended grouping ispkt['__length__'] - (pkt['pad_len'] if … else 0).ContinuationFrame.fragment,UnassignedFrame.dataandGoawayFrame.debuguse the plainpkt['__length__']with no conditional and are the controls that isolate the shape — CONTINUATION returnsb'frag!'correctly.Filed as #668 and deliberately left out of this PR: its fix changes parse output for essentially every HTTP/2 capture, so it wants its own review and its own regression tests rather than riding along on a flags change. It is also why the #652 test asserts the flags octet rather than whole-frame byte equality — a DATA round trip stays lossy for that separate reason until #668 lands, and claiming byte-level round-trip fidelity here would have been false.
Labels, and why
breakingfix+breaking.breakingis additive and its description is "Alters public API or wire output", and this alters output twice over: the reconstructed DATA frame's flags octet changes (0x00→0x01where END_STREAM was set), and a flagless frame's dumped__value__changes from the number0to the string"Flags::None [0]". Both are corrections of wrong output rather than API changes, but both are visible to anyone diffing dumps or reconstructed bytes — and #634 set the precedent for exactly this second change by carrying both labels.Housekeeping
Sample captures were regenerated with
examples/generators/make_samples.py— a fresh worktree has none, and without them 5 runtime-tier tests intests/protocols/application/fail onFileNotFoundErrorforhttp.pcap, unrelated to this change.examples/captures/was not deleted;dhcp.pcapngandin.pcapare committed fixtures. No changelog bullet on this branch — it goes to #657.No file owned by another open PR or live worker was touched:
protocol.py,http.py,extraction.py,interface/core.py,corekit/io.py,hip.py,logging.py,ipv6_route.py,dumpkit/common.py,transport/tcp.pyand.github/**are all untouched.