Skip to content

fix(protocols): makers name the rejected argument in their error messages - #2404

Closed
Qiu-LingYan wants to merge 1 commit into
JarryShaw:mainfrom
Qiu-LingYan:fix/maker-invalid-argument-messages-2296
Closed

Qiu-LingYan wants to merge 1 commit into
JarryShaw:mainfrom
Qiu-LingYan:fix/maker-invalid-argument-messages-2296

Conversation

@Qiu-LingYan

Copy link
Copy Markdown

Fixes #2296.

Per the issue, _make_* builders that reject a bad argument reported the reader's parse-error text (invalid format), which made it impossible to tell which argument was at fault. This PR aligns every maker-side guard with the convention already used elsewhere (e.g. MH._make_opt_mnp's invalid mobile network prefix: {prefix!r}):

invalid <argument>: <requirement>, not <value!r>

Reader-side messages (which legitimately keep invalid format) are untouched.

Changed sites

pcapkit/protocols/transport/sctp.py

  • 11 chunk-flags guards: invalid flags: must be one octet, not {flags!r} (was invalid format)
  • _make_chunk_data: invalid data: must not be empty (was invalid format)
  • _make_param_hostname: invalid name: must be NUL-terminated, not {name!r}
  • _make_cause_invalid_stream: invalid reserved: must be two octets, not {reserved!r}

pcapkit/protocols/internet/mh.py

  • make() unsupported-type guard: invalid data: unsupported type {data.__class__.__name__} (also avoids the type parameter shadowing the builtin)
  • _make_opt_pad Pad1 fallback warning: invalid length for Pad1: {length}; falling back to PadN
  • _make_opt_bad: invalid data: length must be a multiple of 8, not {len(data)!r}
  • _make_opt_auth: invalid data: length must make the option a multiple of 4, not {len(data)!r}
  • _make_opt_badf: invalid data: must not be empty, not {data!r}

pcapkit/protocols/transport/tcp.py

  • _make_mptcp_capable: the prefix-ordering guard now names the first out-of-order field and the one it requires: invalid fields: {name!r} given without {prev!r}

pcapkit/protocols/internet/hip.py

  • nested _make_locator (via _make_param_locator_set): invalid locator: must be an IPv6Address or LocatorData, not {value!r}

pcapkit/protocols/misc/pcapng.py

  • _make_block_dsb: invalid secrets_data: unsupported type {type(secrets_data).__name__!r}

Tests

New tests/protocols/misc/test_maker_invalid_argument_message_2296_unit.py — 12 tests asserting the full message strings (no substring matching), exercising each guard through its public path (e.g. SCTP._make_chunk_*, proto._make_mptcp_capable(skey=None, rkey=...), the HIP parameter-set path with a bad inner locator value, and PCAPNG._make_block_dsb with an unsupported type).

  • new tests: 12 passed
  • targeted regression (SCTP/MPTCP/HIP roundtrip & edge suites): 56 passed + 699 subtests
  • full tests/protocols/transport + internet + misc suite: 1142 passed + 21433 subtests, 0 failures (7 sample-capture FileNotFoundErrors in one run were environmental — captures are generated, not committed — and those 7 tests pass after python examples/generators/make_samples.py)
  • ruff check on the touched production files: unchanged from main baseline

@github-actions github-actions Bot added this to the PyPCAPKit v1.5 milestone Oct 28, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 28, 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: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner

Review: NEEDS CHANGES at 9492f7a38 (Sonnet, read-only cross-review).

Verified good: every change is maker-side only (AST scan, reader-side invalid format sites unchanged 236/236), no exception type changed, all issue checklist sites covered, unit tier green (6500 passed/0 failed across 7 shards), mypy unchanged.

Three things need fixing:

  1. MH._make_opt_auth's new message doesn't match what the guard actually checks — (len(data) + 6) % 4 != 0 rejects len(data) ≡ 2 (mod 4) (lengths 1, 5, 9 → rejected; 2, 6, 10 → accepted), not "length must make the option a multiple of 4." Suggest something like "length + 6 must be a multiple of 4, not N."
  2. The new tests under-pin the fix — reverting 10 of the 11 SCTP invalid flags guards back to invalid format (all but _make_chunk_init) still passes, and so does breaking the TCP predecessor-name lookup. Each of the 11 SCTP sites and the TCP field-pair logic needs its own assertion, not one representative case.
  3. The PR body's "aligns all maker-side guards" overstates what's covered — HOPOPT._make_opt_pad, IPv6_Opts._make_opt_pad (twins of the MH Pad1 warning this PR did fix), HTTP.make, and PCAPNG.make are still on the generic invalid format text and aren't in fix(protocols): makers reject bad arguments with the reader's 'invalid format' text #2296's checklist either. Either convert them too or narrow the body's claim and file a follow-up for them.

Non-blocking: the Pad1 warning and SCTP "must not be empty" message could use a , not <value> tail like _make_opt_badf has; the PR body shows !r forms the code doesn't actually use; the body doesn't follow .github/PULL_REQUEST_TEMPLATE.md.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner

Closing in favor of #2405, which fixes the same issue (#2296) and is already at good-to-go — see the findings above for why this one needed another pass. Thanks for picking up #2296, @Qiu-LingYan — genuinely appreciate the contribution, the timing just didn't line up. Happy to look at future PRs.

@JarryShaw JarryShaw closed this Oct 28, 2026
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 28, 2026
@github-actions github-actions Bot removed this from the PyPCAPKit v1.5 milestone Oct 28, 2026
@JarryShaw JarryShaw removed the review: needs-changes Cross-review at the current head says changes are required; see the verdict comment label Oct 28, 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) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(protocols): makers reject bad arguments with the reader's 'invalid format' text

2 participants