Skip to content

fix(protocol): raise StructError from _read_binary at end of stream (#2292) - #2368

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2292-read-binary-eof
Oct 27, 2026
Merged

fix(protocol): raise StructError from _read_binary at end of stream (#2292)#2368
JarryShaw merged 1 commit into
mainfrom
fix/2292-read-binary-eof

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — bug fix

Description

Closes #2292.

  • Fix: _read_binary on an exhausted stream now raises StructError('unpack: empty buffer', quiet=True, eof=True) instead of a bare TypeError from ord() on an empty read. A size of 0 or less still returns '' without reading.
  • Fail-without-fix: 1 failed on origin/main. Passing on the rebased head.

@JarryShaw
fix(protocol): raise StructError from _read_binary at end of stream (#…
bbd3da9 
…2292)

Problem: _read_binary called ord() on each read(1) without checking an
octet arrived, so an exhausted or short stream raised TypeError where
_read_unpack raises StructError.

Fix: read size octets once; raise StructError with eof set on an empty
read and StructError on a short one, as _read_unpack does, and document
Raises.

Test: test_read_binary_raises_struct_error_at_end_of_stream.
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 27, 2026
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 27, 2026
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at bbd3da9c3 (Sonnet).

  • Confirmed: _read_binary on a truly exhausted stream now raises StructError(..., eof=True) silently; a short-but-nonempty read still raises with eof=False and a CRITICAL log, matching fix(protocol): raise on a short _read_unpack for every size (#2279) #2349's _read_unpack. 1,248-case differential run against the old implementation: 0 mismatches outside the intended fix. No caller relied on the old TypeError. Fail-without-fix: 1 failed on origin/main. On the head, 218+219 passed, including a clean cherry-pick onto current main.
  • Non-blocking: the new test doesn't pin quiet=True, the EOF message text, or the max(size, 0) guard — five mutants the reviewer tried still pass it. Worth tightening in a follow-up, not blocking this one.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@JarryShaw

JarryShaw commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

Ready to merge.

Review: GOOD TO GO (Sonnet) at bbd3da9c3.
CI: Required checks passed, 0 failures (only Sync Status and the non-blocking experimental GraalPy leg still pending).
Ordering leg: protocols clean — 1969 test(s), 0 failure(s), 0 error(s).
Mergeable, 0 unresolved review threads.

@github-actions

github-actions Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Coverage: 91.58% (unit tier, Python 3.15, bbd3da9c3, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19601 881 2620 867 92.12%
pcapkit/corekit 2596 74 818 37 96.51%
pcapkit/dumpkit 282 3 102 3 98.44%
pcapkit/foundation 3029 23 1050 25 98.82%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17456 218 4600 185 98.15%
pcapkit/toolkit 914 36 334 10 95.51%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 4432 1988 1018 167 51.87%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit bd22e9a into main Oct 27, 2026
68 checks passed
@JarryShaw
JarryShaw deleted the fix/2292-read-binary-eof branch October 27, 2026 15:01
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 27, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 27, 2026
@JarryShaw JarryShaw mentioned this pull request Oct 28, 2026
Merged
4 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix)

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.5

Development

Successfully merging this pull request may close these issues.

fix(protocol): _read_binary on an exhausted stream raises TypeError, not StructError

1 participant

@JarryShaw