Skip to content

test(http): expect #811's negative-length ProtocolError, not #814's generic wrap - #823

Closed
JarryShaw wants to merge 1 commit into
mainfrom
fix/822-goaway-negative-length-assertion
Closed

JarryShaw wants to merge 1 commit into
mainfrom
fix/822-goaway-negative-length-assertion

Conversation

@JarryShaw

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

main was red on test_guess_version_reports_a_preface_with_no_frame_as_such's GOAWAY case: it asserted str(exception) == 'HTTP/2: invalid format' (#814's generic wrap), but #811 landed after #814's branch point and fixes that exact negative-length case at its actual root — FieldBase.length now raises ProtocolError itself instead of a bare struct.error. http.py's except ProtocolError: raise then passes that more specific error through unchanged rather than reaching the wrap that produced #814's message. The assertion was stale, not the behaviour, so this updates it to the field-level message while keeping every other check #814 cared about (still a catchable BaseError, still not a bare struct.error, still chained via __cause__).

No production code changed. Verified the failure on stock 3cbdf8999 and the fix passing on CPython 3.14 and 3.10 (pcapkit.__file__ checked in both). tests/protocols/application/ + tests/corekit/ otherwise pass (338 passed, 16 skipped); the 5 test_http_runtime.py failures seen before regenerating samples were pre-existing and unrelated (missing generated captures, fixed by examples/generators/make_samples.py). Coverage of http.py (98%) and field.py (75%) is unchanged since no source line moved.

Closes #822

…eneric wrap

- test_guess_version_reports_a_preface_with_no_frame_as_such's GOAWAY case
  asserted str(exception) == 'HTTP/2: invalid format', which #814 produced
  when the sixteen-octet GOAWAY's oversized declared length drove the
  ``debug`` field negative and struct.calcsize raised a bare struct.error.
- #811 (fix(fields): raise ProtocolError, not struct.error, on a negative
  resolved field length) landed after #814's branch point and fixed that
  exact case at its root: FieldBase.length now raises ProtocolError itself,
  so http.py's ``except ProtocolError: raise`` passes it through unchanged
  instead of reaching the ``except (ValueError, struct.error)`` wrap that
  produced #814's message. The assertion was stale, not the behaviour.
- Update the assertion to the field-level message ("Field debug resolved to
  a negative length; template='-1s'") while keeping every other check #814
  cared about: still a catchable BaseError, still not a bare struct.error,
  still chained to the original struct.error via __cause__.

Verified the failure on stock 3cbdf89 (CPython 3.14) and the fix passing
on both 3.14 and 3.10; tests/protocols/application/ and tests/corekit/
otherwise pass (338 passed, 16 skipped -- 5 unrelated pre-existing failures
in test_http_runtime.py were just missing generated sample captures, fixed
by running examples/generators/make_samples.py). No production code
changed, so coverage of pcapkit/protocols/application/http.py (98%) and
pcapkit/corekit/fields/field.py (75%) is unchanged.

Closes #822
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) test Pull requests that add or correct tests (test: subject prefix) ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 26, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

CI evidence that this fix works, and that the 2 remaining red legs are not this PR's.

On main's run the failing set is 10 legs — all five Pythons × (Unit, Engines) — on test_guess_version_reports_a_preface_with_no_frame_as_such. On this branch that test no longer appears in any failure. What remains is 2 legs so far, and the log names a different test:

#823 Python 3.12 (job 108303494783)
FAILED tests/vendor/test_re_sub_positional_flag_unit.py::RuntimeDeprecationWarningTests::test_positional_flag_shape_would_warn
  AssertionError: DeprecationWarning not triggered

That is #819 — red on 3.10/3.11/3.12 only, fixed by #820, inherited here because this branch is cut from current main which carries both defects. Nothing to do in this PR.

So the board's entire red surface is now two defects with a PR each: #819 → #820, #822 → this. Cross-review is running; verdict to follow.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict: GOOD TO GO (opus; author was sonnet). It tested the judgement call empirically rather than reasoning about it, which is what makes this verdict worth having.

Direction (B) really was rejected on a true premise. The reviewer patched (B) in — deleted the except ProtocolError: raise pass-through at http.py:308-309 — and watched the sibling assertion break:

E  AssertionError: '9-octet frame header' not found in 'HTTP/2: invalid format'
tests/protocols/application/test_http_unit.py:634

I verified the mechanism myself: ProtocolError.__mro__ is [ProtocolError, BaseError, ValueError, …], so ProtocolError subclasses ValueError — without that pass-through, every ProtocolError falls into except (ValueError, struct.error) at :318-319 and gets rewrapped. The assertion at :634 is real and untouched by this PR. And no message-level contract was broken: _guess_version's Raises: documents only the type, and read() has no Raises: block at all.

Fix confirmed on both interpreters: FAIL/FAIL on 3cbdf8999, PASS/PASS on 058b9b4e9, 3.14.7 and 3.10.21, with pcapkit.__file__ asserted per tree. tests/protocols/application/ + tests/corekit/ = 343 passed, 0 failed.

One partial refutation, which I confirmed. Two of the three surviving assertions are tautologies inside assertRaises(ProtocolError): assertIsInstance(…, BaseError) is true for any ProtocolError, and assertNotIsInstance(…, struct.error) is true because the two hierarchies are disjoint (measured, with a real struct.error as the discriminating control). Only the __cause__ check has teeth. They were equally tautological under #814, so #823 weakens nothing — but the new comment presents all three as substantive, and that clause overstates.

Your indentation observation was right and is not a gate. Line 663 sits at column 27 against a visual indent of 26 — pycodestyle E127. But nothing here would catch it: every lint recipe passes only pcapkit (Makefile:187,190,193), tests/ reaches none of them, all six lint.yml steps carry continue-on-error: true, and pylint 4 removed bad-continuation entirely. Not invented, not blocking.

Three follow-ups filed rather than folded in here: #824 and #825. The 5 red legs are #819's, inherited — see above. I am holding the good-to-go label until CI completes (2 legs still running); the review itself is done.

@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 26, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

CI is complete — ok=21 fail=6 inc=0 — so the gate I was holding is satisfied and this is review: good-to-go on 058b9b4e9.

The decisive evidence that the fix works, rather than just that fewer things are red: Python 3.13, 3.14, Engines 3.13 and Engines 3.14 are all SUCCESS. Those are exactly the legs main fails on #822's defect, and #819 does not reach them.

All 6 remaining failures are #819, one defect, confirmed from two logs rather than extrapolated from one:

Python 3.12        (job 108303494783)  FAILED …test_re_sub_positional_flag_unit.py::…test_positional_flag_shape_would_warn
Engines Python 3.10 (job 108303494787) FAILED …test_re_sub_positional_flag_unit.py::…test_positional_flag_shape_would_warn

Same test, same assertion, on 3.10/3.11/3.12 × (Unit, Engines) only — inherited from main and fixed by #820. Nothing to do here.

JarryShaw added a commit that referenced this pull request Sep 26, 2026
…k comment (#824)

The fallback-arm comment in `_guess_version` (roughly :358-395) documented
measurements from before #799 and #811 landed, so three sentences described
behaviour the code no longer has:

- `HTTP(io.BytesIO(b'\x00' * 8), 8)` was cited as raising a bare
  `struct.error`; #799's nine-octet guard now answers `ProtocolError:
  unknown HTTP version` instead, re-measured on this tree.
- The 16-octet `GOAWAY` example was cited as still raising a bare
  `struct.error` through `httpv2.HTTP` directly, with the fix left as future
  work "tracked as #805". #811 closed that at `FieldBase.length`; the same
  input now raises `ProtocolError: Field debug resolved to a negative
  length; template='-1s'`.
- Both sentences are rewritten in the past tense with the current, re-measured
  outcome, keeping the surrounding causal explanation (why the last arm still
  suppresses `struct.error`, as defence in depth) intact.

Comment-only change, no behaviour touched. Verified `tests/protocols/application/`
still passes (122 passed, 1 pre-existing failure: #822's
`test_guess_version_reports_a_preface_with_no_frame_as_such`, confirmed failing
identically on stock `origin/main`, fixed by open PR #823 rather than here).

Closes #824
@JarryShaw

Copy link
Copy Markdown
Owner Author

Landed on main by direct push, at the maintainer's instruction — closing as delivered rather than merged.

main was red on 10 legs and the ruling was "can be fixed with a direct push there". This PR's commit is now 21e9588af on main, and its content is byte-identical to what was reviewed here (diff of the two diffs is empty). #822 auto-closed from the commit's Closes #822 trailer.

Verified before pushing, on top of 319da66bb: the test failed first (1 failed, 58 deselected), then 59 passed, 388 subtests passed after the cherry-pick. The cross-review verdict on this content stands — GOOD TO GO (opus).

main now carries both test fixes (#820's as 319da66bb, this as 21e9588af) and should be green.

@JarryShaw JarryShaw closed this Sep 26, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) ci Pull requests that change CI or workflow configuration (ci: subject prefix) test Pull requests that add or correct tests (test: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

test: main is red on all five Pythons — #811's negative-length guard pre-empts #814's HTTP/2 struct.error assertion

1 participant