protocols: bind HTTP.make to a real instance, and wrap SETTINGS' item schema

[JarryShaw]

Summary

Two single-site application-layer fixes, batched because both are HTTP-family corrections in files nobody else owns, not because they are related.

#452 — HTTP.make calls the versioned make unbound on the class

pcapkit/protocols/application/http.py:134 (now :147) was:

return protocol.make(**kwargs)   # protocol is the class, make is an instance method

protocol is the imported class (httpv1.HTTP or httpv2.HTTP), and make is an ordinary instance method on both — httpv1.py:147 is def make(self, ...), matching the abstract ProtocolBase.make(self, **kwargs) at protocol.py:258. inspect.getattr_static(HTTPv1, 'make') returns a plain function, confirming it is neither staticmethod nor classmethod. So the call left self unfilled and raised TypeError for every real call.

Corrected reproduction (the issue's original one called HTTP.make(version=1, ...) directly on the class, which fails at the outer HTTP.make — itself an ordinary instance method — and proves nothing about the inner dispatch; see the correction posted on the issue). The real, supported entry point is construction, since ProtocolBase.__init__'s **kwargs-only overload (protocol.py:517) calls self.pack(**kwargs), and pack (protocol.py:284) is self.__header__ = self.make(**kwargs) — a bound call on the outer HTTP instance that reaches the inner, previously-unbound protocol.make(**kwargs):

main      HTTP(version=1, http_version='1.1', method='GET', uri='/')
          -> TypeError: HTTP.make() missing 1 required positional argument: 'self'

patched   same call
          -> ProtocolError: HTTP/1: invalid format

The second is an in-library error raised inside HTTPv1.make from deliberately minimal keyword arguments — proof the dispatch now reaches the callee and the callee is validating, not merely the absence of a TypeError.

Fix and why: protocol.__new__(protocol).make(**kwargs) — construct a bare instance of the versioned class (bypassing __init__'s parse/pack machinery via __new__) and call the now-bound make on it. This is a third option beyond the two the issue named (construct through read's protocol(self._data, length, **kwargs) path, or make the versioned make a classmethod/staticmethod):

• A full protocol(**kwargs) construction doesn't fit: with no file, __post_init__ would call self.pack(**kwargs) (which calls make anyway) and then also self.unpack(...) → self.read(...), parsing the bytes right back — a full pack-then-reparse round trip when only the pack half was asked for, and a different, heavier operation than "construct and call make."
• Making the versioned make a classmethod/staticmethod changes a signature shared with the abstract ProtocolBase.make(self, **kwargs), and HTTPv2.make genuinely needs instance access (self.__frame__, self._lookup_registry, getattr(self, meth_name, ...)), so it isn't a drop-in classmethod without deeper rework.
• protocol.__new__(protocol) works today because neither HTTPv1.make nor HTTPv2.make reads any state that __init__/__post_init__ establishes (HTTPv1.make only calls self._make_index, already a classmethod; HTTPv2.make only reads self.__frame__, a class-level registry). What would break it: a future make override on either class that reads instance state set up during __init__ (e.g. self._data, self._info) would need a different dispatch here.

I checked whether this file's existing unit tests use this same object.__new__ idiom as precedent and want to be precise rather than overclaim: test_http_read_make_and_guess_version_delegation_paths uses @staticmethod fakes, which is exactly what hid this bug and is not precedent. Other tests in the same file (test_httpv1_id_make_data_and_request_construction, test_httpv2_id_length_make_bytes_and_register_frame_warning) do call object.__new__(HTTPv1)/object.__new__(HTTPv2) on the real classes before calling .make(...) directly, so the specific __new__-bypass technique does have real-class precedent in this file, even though no existing test exercises it through HTTP.make's own dispatch.

#459 — SettingsFrame.settings passes a Schema class where a field belongs

pcapkit/protocols/schema/application/httpv2.py:

settings: 'list[SettingPair]' = ListField(
    length=lambda pkt: pkt['__length__'],
    item_type=SettingPair,  # type: ignore[arg-type]
)

SettingPair is a Schema subclass (a SchemaMeta), not a field instance. Confirmed the defect directly: with the bare class, ListField.unpack's schema branch does field = self._item_type(packet) → constructs a SettingPair from the packet dict instead of configuring a per-item field, and the isinstance(self._item_type, SchemaField) check (which selects the schema branch) is False for a bare class in the first place — so it falls into the plain-field branch and fails there: AttributeError: 'SettingPair' object has no attribute 'length'.

Fix: item_type=SchemaField(schema=SettingPair), matching every sibling ListField (tcp.py's SACK.sack, hip.py, mh.py's CGAParametersOption.parameters, sctp.py's gap_blocks/dup_tsn), and the # type: ignore[arg-type] is removed. mypy confirms the removal is clean: 124 errors in 40 files at da2422728, identical before and after this change (baseline measured fresh at the actual base commit — the issue's cited "128 errors in 41 files" was measured 4 commits earlier at e2d8ed6d1 and has since drifted for unrelated reasons). Neither this file nor http.py appears anywhere in the mypy output, before or after.

What stays unobservable until PR #457 lands: #459 is masked by #445 — the SETTINGS frame's pack path dies earlier with KeyError: 'flags' — raised by FrameType.post_process at schema/application/httpv2.py:144, where it reaches the enclosing header's flags field through a nested packet context that cannot see it — before ever reaching the settings field. Confirmed via tests/protocols/test_option_roundtrip_unit.py: the httpv2-frame/SETTINGS EXPECTED_FAILURES entry still fails the exact same recorded way (CONSTRUCT: KeyError: 'flags') with this fix applied — no EXPECTED_FAILURES entries changed or need to change. So the regression test here is a unit-level assertion directly against the field wiring (SettingsFrame.__fields__['settings']) and a real SettingPair unpack over hand-built bytes, not an end-to-end HTTPv2 round trip — that remains blocked on #457.

Regression tests

Both new tests deliberately exercise real classes rather than fakes, since fakes with a compatible staticmethod/__call__ are what hid each defect:

• test_http_make_dispatches_to_real_versioned_classes and test_http_construction_reaches_the_versioned_make_callee (the corrected, construction-based reproduction) in tests/protocols/application/test_http_unit.py, against the real HTTPv1/HTTPv2 classes.
• test_settings_frame_settings_field_wraps_item_schema, against the real SettingsFrame.__fields__['settings'] field and SettingPair schema.

Before/after, quoted:

#452, construction-based repro, before fix:
TypeError: HTTP.make() missing 1 required positional argument: 'self'
# after fix:
ProtocolError: HTTP/1: invalid format   (proves the callee was reached and is validating)

# 459, field-wiring assertion, before fix:
AssertionError: <class '...httpv2.SettingPair'> is not an instance of <class '...misc.SchemaField'>
# after fix: passes; unpacking two hand-built SETTINGS pairs (6 octets each) produces
# two correctly-typed SettingPair instances (id=1/value=4096, id=2/value=0). A harmless
# SchemaWarning ("packet length < 0") fires from SchemaField's -1 default-length
# sentinel -- the same behaviour mh.py's CGAParametersOption already has -- and does
# not affect the parsed values.

Test plan

• --collect-only: 994 collected at base da2422728 (corroborated independently by PR schema: a forward match consumes nothing, so bill it nothing in len(schema) #456's and PR protocols: floor CALIPSO, MPL and REG_INFO's wire-derived lengths at zero #460's agents against the same base).
• Full suite passes on the patched branch, fixtures generated once via examples/generators/make_samples.py beforehand — result posted as a PR comment once the run lands.
• tests/protocols/test_option_roundtrip_unit.py: all 7 tests / 299 subtests pass; httpv2-frame/SETTINGS's recorded failure is unchanged. No EXPECTED_FAILURES changes.
• mypy: 124 errors in 40 files at da2422728, unchanged by either fix; the removed # type: ignore[arg-type] produces no new "unused ignore" or other error.
• Both regression tests independently confirmed to fail (with the exact quoted text above) against the unpatched base, and pass against the fix.

Branched from da2422728 (base origin/main has since advanced to 94a93e721, a docs-only pep.rst delivery-sequence addition that does not touch code, so the base was not moved to chase it).

Closes #452
Closes #459

[JarryShaw]

After-suite result, on this branch's tip 804ca299d (patched, fixtures generated once beforehand via examples/generators/make_samples.py):

980 passed, 17 skipped, 1267 subtests passed in 640.44s (0:10:40)

997 collected (994 established baseline at da2422728 + 3 new regression tests added by this PR: test_http_make_dispatches_to_real_versioned_classes, test_http_construction_reaches_the_versioned_make_callee, test_settings_frame_settings_field_wraps_item_schema). 980 + 17 = 997, consistent.

Run with PYTHONSAFEPATH=1, PYTHONPATH at the worktree, interpreter .venv/bin/python 3.14.7.

Against the established 994 collected / 977 passed / 17 skipped baseline at da2422728: net +3 passed, 0 skipped, 0 failed — exactly the 3 new tests, all green.

[JarryShaw]

Standing in for Copilot (out of tokens) on this review. Reviewed at head 804ca299d, fetched via refs/pull/462/head and confirmed against the reported sha. Read all code from git show <ref>:<path> against that ref and origin/main, never from an ambient working tree.

CI

All 23 checks settled green: 21 SUCCESS, 2 SKIPPED (Docs test gate and Gate (full suite, Python 3.14) — both skip on every recent merged PR here too, e.g. #460 and #437, so that's normal for this repo, not a gap). No red checks at any point.

Being 3 behind origin/main

git rev-list --count 804ca299d..origin/main = 3 (94a93e721 docs-only, 489eef651 MH registry work, faf86d26b CALIPSO/MPL/REG_INFO length floors). None of the three touches http.py, httpv2.py's schema, or test_http_unit.py — confirmed by git show --stat on each. gh pr view --json mergeable,mergeStateStatus reports MERGEABLE/BEHIND. Being behind doesn't matter here.

#452 — the __new__ fix (the part worth real scrutiny)

Verified the corrected reproduction directly: HTTP.make(version=1, ...) fails at the outer HTTP.make on both trees (proves nothing); HTTP(version=1, http_version='1.1', method='GET', uri='/') raises TypeError: HTTP.make() missing 1 required positional argument: 'self' on da2422728 and ProtocolError: HTTP/1: invalid format on 804ca299d — matches the issue's correction comment exactly.

Traced the invariant the fix depends on: read HTTPv1.make, HTTPv2.make, and every _make_http_* handler reachable from HTTPv2.make. The only self. accesses anywhere in that graph are self._make_index (@classmethod), self._lookup_registry (@staticmethod), the class-level self.__frame__ registry, and bound-method lookups — nothing touches what ProtocolBase.__init__/__post_init__ establish. Also checked the rejected alternative: ProtocolBase.__post_init__ (protocol.py:654-676) unconditionally does self.pack(**kwargs) and then self._info = self.unpack(...) when file is None, so constructing normally really would pack-then-reparse, confirming the PR's stated reason for rejecting it. Full reasoning posted as an inline comment on http.py:150.

My opinion on protocol.__new__(protocol).make(**kwargs): it's the right call. It's narrower than either alternative the issue named, it's backed by a code comment that correctly states its own precondition and what would violate it, and the failure mode if that precondition ever breaks is a loud AttributeError (since protocol.__new__ still runs ProtocolBase.__new__, unlike the object.__new__ this file's own existing tests use) rather than a silent wrong result. I would merge this as written.

Test-double trap, checked: test_http_read_make_and_guess_version_delegation_paths does use @staticmethod fakes (confirmed by reading it) and does not serve as precedent — verified by running it unmodified against both trees; it passes on both regardless of the bug, exactly as the PR says. The two new regression tests (test_http_make_dispatches_to_real_versioned_classes, test_http_construction_reaches_the_versioned_make_callee) import the real HTTP/HTTPv1/HTTPv2 classes with no mocking of make — confirmed by reading the imports and by running them against the unpatched base, where both fail with the exact quoted text, and against the patched head, where both pass. The PR body's precedent claim about the other two existing tests (test_httpv1_id_make_data_and_request_construction, test_httpv2_id_length_make_bytes_and_register_frame_warning) is also accurate on inspection, with one nuance: those use object.__new__(HTTPv1)/object.__new__(HTTPv2), not <class>.__new__(<class>) as the actual fix does — a real but immaterial difference (the fix's form is the more conservative of the two, since it still runs ProtocolBase.__new__).

#459 — SchemaField(schema=SettingPair)

Confirmed the diff is exactly item_type=SettingPair, # type: ignore[arg-type] -> item_type=SchemaField(schema=SettingPair),. Confirmed every sibling ListField wrapping a Schema subclass does the same (tcp.py:393, hip.py:287, mh.py:535, sctp.py:702) and none of the ones left bare wrap a Schema subclass either. mypy: ran it myself at both 804ca299d and da2422728 — 124 errors in 40 files at both, zero-diff on the error sets, neither http.py nor httpv2.py's schema module appears in either run. Matches the claim exactly.

The masking claim — checked, and found one inaccuracy. Reproduced constructing and packing a SETTINGS frame directly against this branch: it dies in FrameType.post_process (httpv2.py:144) with KeyError: 'flags', before ever reaching the settings field — matching EXPECTED_FAILURES['httpv2-frame/SETTINGS'] in test_option_roundtrip_unit.py exactly (Gap('CONSTRUCT', "KeyError: 'flags'", ...)), which I also confirmed is unmodified by this PR (git diff on that file between da2422728 and 804ca299d is empty). Ran tests/protocols/test_option_roundtrip_unit.py: 7 passed, 299 subtests passed — matches the claimed count exactly. So the substantive claim holds: #459 really is unreachable end-to-end until #457 (confirmed still OPEN) lands, and no EXPECTED_FAILURES entry needs to change.

But the PR description and the new test's docstring both say this masking error is KeyError: 'length' — that's wrong; it's KeyError: 'flags', as the PR's own next sentence correctly quotes. Looks like it got carried over from issue #459's original filing text (and from #445's actual KeyError: 'length' symptom on mh.py's CGAParameter.extensions, a different field entirely) without being re-checked against what httpv2.py actually raises. Flagged inline on the test docstring; doesn't affect correctness anywhere, just the write-up.

Numbers, measured fresh rather than trusted

• mypy at da2422728: 124 errors in 40 files (not 128/41 — that was e2d8ed6d1, four commits earlier).
• pytest --collect-only: 994 at da2422728, 997 at 804ca299d (+3, exactly the new tests).
• tests/protocols/application/test_http_unit.py: 25 passed at head; all 3 new tests independently confirmed to fail on the unpatched base with the exact text the PR quotes, and pass on head.
• Full suite at 804ca299d, fixtures generated once via make_samples.py beforehand, PYTHONSAFEPATH=1/PYTHONPATH at the worktree/.venv 3.14.7 (pcapkit.__file__ printed and confirmed pointing at this checkout): 980 passed, 17 skipped, 1267 subtests passed in 643.50s — matches the PR's own posted after-suite comment exactly, including the subtest count.
• Checked the except ProtocolError: raise / except ValueError ordering PR protocols: real routing type in IPv6-Route diagnostics, and fix HTTP's drained explicit version= buffer #451 depends on in this same file (http.py:105-108): untouched by this diff, which only touches make()'s return statement — make() itself has no such try/except block at all.

Verdict

GOOD TO MERGE at 804ca299d. Both fixes are correct, narrowly scoped, and backed by regression tests that demonstrably fail on the unpatched base and pass on the patch (verified by running them against both, not by reading the assertions). The __new__ dispatch is the right design given the alternatives, is honestly documented including its own failure mode, and I would not ask for a different approach. Two inline comments posted: one is pure design commentary/agreement on http.py:150, the other flags a KeyError: 'length' vs KeyError: 'flags' mismatch between the PR's prose and its own (correct) test/table evidence in test_http_unit.py:853 — neither is blocking.