fix(ftp): reject a command that is not str or bytes with ProtocolError (#2290) by JarryShaw · Pull Request #2365 · JarryShaw/PyPCAPKit

JarryShaw · 2026-10-27T03:55:43Z

Searched for similar pull requests
Followed the coding style
make test passes, and a test case covers the change
Changelog entry: N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

fix — bug fix

Description

Closes #2290.

Fix: FTP.make's command argument is now checked explicitly. str and bytes build as before; bytearray is newly accepted (on main it raised AttributeError, which is one of fix(ftp): make raises a bare AttributeError for a command that is not str or bytes #2290's own repro lines). Anything else raises ProtocolError with the house-form message — including the SimpleNamespace(value=...) shape, which on main was a reachable, working branch (pinned by an existing test), not dead code.
Deviation from fix(ftp): make raises a bare AttributeError for a command that is not str or bytes #2290, flagged to the owner: the issue asks for anything that isn't str/bytes to raise, which would reject bytearray too. I accepted it instead, matching fix(ethernet): a non-string MAC address raises a bare TypeError, not ProtocolError #2222's precedent. Posted to fix(ftp): make raises a bare AttributeError for a command that is not str or bytes #2290 for a ruling.
Fail-without-fix: 6 failed on origin/main. Passing on the rebased head.

JarryShaw · 2026-10-27T04:11:19Z

Review: NEEDS CHANGES at bd94071bd (Sonnet) — prose only, code is correct.

Confirmed: make/FTP(cmmd=...) build correctly for str, bytes, bytearray and Command; nine other shapes (int, bool, float, a SimpleNamespace, a bytes-valued plain Enum, etc.) all raise the house-form message. Nothing outside ftp.py relied on the old .value branch. Fail-without-fix: 6 failed on origin/main. On the head, 46+215 passed.
Required: the PR body's "str/bytes/bytearray build as before" and "dead .value branch" are both false against main — bytearray raised AttributeError there, and the .value branch was reachable (a SimpleNamespace/bytes-valued-Enum built successfully, with a test pinning it). Correct the body.
Required: reword the new test's docstring, which narrates the removed branch.
Real deviation from fix(ftp): make raises a bare AttributeError for a command that is not str or bytes #2290, needs a ruling: the issue asks for bytearray to raise; this PR accepts it (disclosed in the commit message, not the PR body). I'll post that to fix(ftp): make raises a bare AttributeError for a command that is not str or bytes #2290 with a recommendation.

#2290) FTP.make fell back to cmmd.value for anything not str or bytes. Command is a StrEnum, so a member never reached that branch, and every value that did (an int, a bool, a bytearray) raised a bare AttributeError. Accept a bytearray as bytes, and raise "FTP: invalid command: must be a str, bytes or bytearray, not ..." for anything else, a bool included, as #2256 does for ARP addresses. Test: the exact message for int, bool, float and a .value-carrying object, and a bytearray command building like bytes; the make test that pinned the duck-typed .value object now uses a bytearray.

JarryShaw · 2026-10-27T04:12:50Z

Amended bd94071bd → 0603014fa: reworded the new test's docstring and name, and corrected the PR body — bytearray is newly accepted (it raised on main), and the .value branch was reachable and working on main, not dead. Posted the bytearray-acceptance deviation to #2290 for a ruling, with a recommendation to keep it accepted. test_ftp_unit.py + test_docstring_contract.py give 19 passed.

github-actions · 2026-10-27T05:41:23Z

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

Package	Statements	Missed	Branches	Partial	Cover
`pcapkit (top level)`	104	4	20	2	95.16%
`pcapkit/const`	19599	881	2618	867	92.11%
`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`	17442	218	4586	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 · 2026-10-27T05:56:48Z

Ready to merge at 0603014fa. Closes #2290.

Review: NEEDS CHANGES → fixed (PR body overstated two things; test docstring/name reworded) → self-certified, prose-only change. A real design question it raised (bytearray accepted vs. rejected) was ruled by the owner: keep it accepted, matching this branch.
CI: Required checks passed SUCCESS, 0 failures, KTLO clean.
Ordering leg: protocols gave 1969 tests with 0 failures, run with the PR merged into current main. (One log of this run has a NUL-byte artifact from the earlier queue script re-using a filename — harmless; the unittest summary line is clean.)
Merge state: mergeable.

Problem: of the 60 pull requests merged in the #2312-#2407 range, only #2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and #2398 and #2403 only reworded lines that were already there. Every other user-visible change in the range -- wire-semantics fixes, maker guards, reassembly behaviour, public-API and error-message changes -- went unrecorded, so the entry no longer describes the release. Fix: one entry per user-visible pull request, in each module section's existing style and ordered by the issue it closes. Two pairs share a bullet, since each is the direct follow-up of the other: #2393 with #2401, and #2405 with #2407. The diff on the entry file is purely additive. Internal-only work is left out deliberately: dependency and action bumps (#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379, #2380, #2402), the unreachable-branch removal (#2378), and the #: comment marker the HIP Group and Packet crawlers now write (#2381). That last one renders nothing either way: the marker lands inside the generated _missing_ body, conf.py's exclude-members drops _missing_ from autodoc, and a comment in a method body is not an attribute comment to begin with. #2392 and #2403 are recorded together, under Added rather than Fixed, for the declarations they introduce: __proto_key__ on the seven classes owning a __proto__ registry (#2392), __proto_dest__ on the four enum registries and Vendor.PROTO_DEST to generate it (#2403). Both PRs added the autodoc entries, so both are rendered public attributes. #2392's rejection-message change is deliberately not recorded as a fix: the bare "code must be ..." text it prefixed came from _check_code, which #2193 introduced and which is in no release tag -- absent at v1.5.0b14 -- so no released version ever emitted it. #2403 is no longer skipped as internal because it reworded an existing bullet to name __proto_dest__ while nothing credited the attribute's introduction. Four claims are corrected against the source rather than carried over: FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not 1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback read cmmd.value, so a .value-carrying object was accepted unchecked rather than raising; #2399 left the dpkt adapter and the adapters' ModuleNotFound raises alone, so it claims no universal prefix; and #2397's old tail did name the version it got, so the entry quotes it and gains the :issue:`2302` it was missing. #2370's "a Section Header Block ... still fails" sentence is dropped: #2369 settles the byte order first, and an SHB built from raw octets with ctx=None now rebuilds byte-exact in both byte orders. CHANGELOG.md is regenerated with util/changelog_md.py. Test: tests/project/ in full, 0 failures, including test_changelog_md.py's in-step and supported-subset checks; util/changelog_md.py --check reports the two files in step; the entry parses under docutils with no structural warning.

Problem: of the 60 pull requests merged in the #2312-#2407 range, only #2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and #2398 and #2403 only reworded lines that were already there. Every other user-visible change in the range -- wire-semantics fixes, maker guards, reassembly behaviour, public-API and error-message changes -- went unrecorded, so the entry no longer describes the release. Fix: one entry per user-visible pull request, in each module section's existing style and ordered by the issue it closes. Three pairs share a bullet, since each is the direct follow-up or counterpart of the other: #2393 with #2401, #2405 with #2407, and #2392 with #2403. The diff on the entry file is purely additive. Internal-only work is left out deliberately: dependency and action bumps (#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379, #2380, #2402), the unreachable-branch removal (#2378), and the #: comment marker the HIP Group and Packet crawlers now write (#2381). That last one renders nothing either way: the marker lands inside the generated _missing_ body, conf.py's exclude-members drops _missing_ from autodoc, and a comment in a method body is not an attribute comment to begin with. #2392 and #2403 are recorded together, under Added rather than Fixed, for the declarations they introduce: __proto_key__ on each class owning a __proto__ registry (#2392), __proto_dest__ on the enum registries and Vendor.PROTO_DEST to generate it (#2403). The bullet names those classes and registries rather than counting them, per the no-counted-numbers rule for the unreleased entry. Both PRs added the autodoc entries, so both are rendered public attributes. #2392's rejection-message change is deliberately not recorded as a fix: the bare "code must be ..." text it prefixed came from _check_code, which #2193 introduced and which is in no release tag -- absent at v1.5.0b14 -- so no released version ever emitted it. #2403 is no longer skipped as internal because it reworded an existing bullet to name __proto_dest__ while nothing credited the attribute's introduction. Four claims are corrected against the source rather than carried over: FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not 1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback read cmmd.value, so a .value-carrying object was accepted unchecked rather than raising; #2399 left the dpkt adapter and the adapters' ModuleNotFound raises alone, so it claims no universal prefix; and #2397's old tail did name the version it got, so the entry quotes it and gains the :issue:`2302` it was missing. #2370's "a Section Header Block ... still fails" sentence is dropped: #2369 settles the byte order first, and an SHB built from raw octets with ctx=None now rebuilds byte-exact in both byte orders. CHANGELOG.md is regenerated with util/changelog_md.py. Test: tests/project/ in full, 0 failures, including test_changelog_md.py's in-step and supported-subset checks; util/changelog_md.py --check reports the two files in step; the entry parses under docutils with no structural warning.

… (#2414) FTP.make's args branch encoded anything that was not bytes, so a bytearray, int, bool or memoryview raised a bare AttributeError ('bytearray' object has no attribute 'encode') instead of a ProtocolError. The gap predates #2365, which widened the sibling cmmd parameter but left args as it was. Accept a bytes or bytearray verbatim, encode only a genuine str, and raise "FTP: invalid arguments: must be a str, bytes or bytearray, not ..." for anything else, mirroring cmmd's guard from #2365. Widened the args annotation to include bytearray and extended the Args:/Raises: docstring entries. Test: tests/protocols/application/test_ftp_unit.py gains test_ftp_make_rejects_arguments_that_are_not_str_bytes_or_bytearray, pinning the exact message for int, bool, float, memoryview and a .value-carrying object, and a bytearray argument building on both a request and a reply. Confirmed fail-without-fix: with the guard reverted the test fails 6/6 with AttributeError rather than ProtocolError. tests/protocols/ in full: 2021 passed / 51393 subtests / 0 failed, after regenerating examples/captures via examples/generators/make_samples.py. mypy reports nothing on ftp.py and pylint's findings on both touched files are unchanged from main.

Problem: of the 60 pull requests merged in the #2312-#2407 range, only #2334, #2336, #2337, #2340 and #2348 wrote a 1.5.0 changelog entry, and #2398 and #2403 only reworded lines that were already there. Every other user-visible change in the range -- wire-semantics fixes, maker guards, reassembly behaviour, public-API and error-message changes -- went unrecorded, so the entry no longer describes the release. Fix: one entry per user-visible pull request, in each module section's existing style and ordered by the issue it closes. Three pairs share a bullet, since each is the direct follow-up or counterpart of the other: #2393 with #2401, #2405 with #2407, and #2392 with #2403. The diff on the entry file is purely additive. Internal-only work is left out deliberately: dependency and action bumps (#2387 to #2391), test-only and docs-prose passes (#2333, #2376, #2379, #2380, #2402), the unreachable-branch removal (#2378), and the #: comment marker the HIP Group and Packet crawlers now write (#2381). That last one renders nothing either way: the marker lands inside the generated _missing_ body, conf.py's exclude-members drops _missing_ from autodoc, and a comment in a method body is not an attribute comment to begin with. #2392 and #2403 are recorded together, under Added rather than Fixed, for the declarations they introduce: __proto_key__ on each class owning a __proto__ registry (#2392), __proto_dest__ on the enum registries and Vendor.PROTO_DEST to generate it (#2403). The bullet names those classes and registries rather than counting them, per the no-counted-numbers rule for the unreleased entry. Both PRs added the autodoc entries, so both are rendered public attributes. #2392's rejection-message change is deliberately not recorded as a fix: the bare "code must be ..." text it prefixed came from _check_code, which #2193 introduced and which is in no release tag -- absent at v1.5.0b14 -- so no released version ever emitted it. #2403 is no longer skipped as internal because it reworded an existing bullet to name __proto_dest__ while nothing credited the attribute's introduction. Four claims are corrected against the source rather than carried over: FTP_DATA shipped in pcapkit/protocols/application/ftp.py from 1.0.0, not 1.4.0 (class FTP_DATA(Raw) is there at v1.0.0); #2365's pre-fix fallback read cmmd.value, so a .value-carrying object was accepted unchecked rather than raising; #2399 left the dpkt adapter and the adapters' ModuleNotFound raises alone, so it claims no universal prefix; and #2397's old tail did name the version it got, so the entry quotes it and gains the :issue:`2302` it was missing. #2370's "a Section Header Block ... still fails" sentence is dropped: #2369 settles the byte order first, and an SHB built from raw octets with ctx=None now rebuilds byte-exact in both byte orders. CHANGELOG.md is regenerated with util/changelog_md.py. Test: tests/project/ in full, 0 failures, including test_changelog_md.py's in-step and supported-subset checks; util/changelog_md.py --check reports the two files in step; the entry parses under docutils with no structural warning.

JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 27, 2026

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

github-project-automation Bot added this to PyPCAPKit Oct 27, 2026

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 27, 2026

JarryShaw force-pushed the fix/2290-ftp-make-command-type branch from bd94071 to 0603014 Compare October 27, 2026 04:12

JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 27, 2026

JarryShaw moved this to In review in PyPCAPKit Oct 27, 2026

JarryShaw merged commit 7c4d7ce into main Oct 27, 2026
63 checks passed

JarryShaw deleted the fix/2290-ftp-make-command-type branch October 27, 2026 14:57

github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 27, 2026

JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 27, 2026

Navigation Menu

Repository navigation

fix(ftp): reject a command that is not str or bytes with ProtocolError (#2290) - #2365

fix(ftp): reject a command that is not str or bytes with ProtocolError (#2290)#2365
JarryShaw merged 1 commit into
mainfrom
fix/2290-ftp-make-command-type

JarryShaw commented Oct 27, 2026 •

edited

Loading

Uh oh!

JarryShaw commented Oct 27, 2026

Uh oh!

JarryShaw commented Oct 27, 2026

Uh oh!

github-actions Bot commented Oct 27, 2026

Uh oh!

JarryShaw commented Oct 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

JarryShaw commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of your pull request?

Description

Uh oh!

JarryShaw commented Oct 27, 2026

Uh oh!

JarryShaw commented Oct 27, 2026

Uh oh!

github-actions Bot commented Oct 27, 2026

Uh oh!

JarryShaw commented Oct 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

JarryShaw commented Oct 27, 2026 •

edited

Loading