fix(protocol): refuse a construction keyword no signature declares (#617) - #640
Conversation
58a9f82 to
b9220c2
Compare
GOOD TO GO — after one required documentation fix, now appliedCross-review by an independent Sonnet subagent (this PR was raised by an agent, so it was reviewed by a different model), briefed to falsify rather than to bless and run read-only. It worked from its own
What it disputed, and what changed as a result1. A direct
Fixed in the amended commit ( 2. The reviewer's count of the Where I disagree with the reviewer
They inherit from What it independently confirmed
What it could not verifyThe 322-case round-trip total (it verified the 45-entry table instead), the "54 failures on the naive version" figure (a claim about an implementation never in this diff), Its judgement on the two arguable calls
The amended commit is |
b9220c2 to
fbbc137
Compare
Rewritten: changelog entry moved out, rebased onto current
|
55f6e57 to
c46f67b
Compare
) **Behaviour change to a public API.** Building a protocol through its constructor with a keyword that names nothing now raises `UnsupportedCall` instead of discarding it. * Every `make` in the tree ends its signature with `**kwargs` and reads nothing out of it, so a misspelled keyword was accepted, dropped, and the field it named kept its default -- wrong octets, with nothing said. That is what #602 cost: `seq=1` where `TCP.make` spells the parameter `seq_no`, and 25 fixture frames carried sequence number `0` against an empty `warnings` list. The schema layer already warns `UnknownFieldWarning` for a field it does not know; this closes the asymmetry from the other end. * Checked in `ProtocolBase.__init__`, against the union of every keyword-taking parameter of `make`, `read`, `pack`, `unpack`, `__post_init__` and `__init__` across the MRO -- wider than `make` alone because `__post_init__` hands one `**kwargs` to the construction *and* the parse, so `HIP.read`'s `extension` legitimately travels through `HIP.make`. Parsing is untouched, and a direct `SomeProtocol.make(...)` call is not covered -- documented, and pinned by a test. * New `ProtocolBase.__keywords__`: a set for a keyword read out of `**kwargs` by name, `None` for a dispatcher that cannot enumerate its own. `HTTP` is the one user of `None`; it is not inherited, so `HTTPv1`/`HTTPv2` stay checked. * Reading that attribute needs an absent-versus-`None` marker, and it is `_Absent`, an instance of a `@final` `_AbsentType` carrying `__bool__` and `__repr__`, rather than a bare `object()`. `None` is unavailable because it is the opt-out above, and `NoValue` is not reused: it is documented as the value of `FieldBase.default`, and `protocol.py` imports nothing from `corekit.fields` today. * `from_data` warns rather than raises -- its keywords come from `_make_data`, not from a caller. That makes three latent defects audible instead of fatal: `Frame` returns `ts_src` for `ts_sec`, `L2TPv2` `prio` for `priority`, and `Header` a `magic_number` `make` does not take. * Fixed the four misspellings this surfaced, all #602 residue: `_TCP_BASE` in `examples/generators/dispatch.py` and three stale copies under `tests/protocols/transport/`. Fixes #617. New `tests/protocols/test_construction_keyword_check_unit.py`: 22 tests, 37 subtests, of which 15 fail on `main` for the keyword check and 2 more for the sentinel. Two further sentinel tests check the behaviour rather than the marker's shape, and catch the mistake a swap like this actually makes -- comparing against a second instance of the right type fails them with `TypeError: '_AbsentType' object is not iterable`. The sentinel's six statements are covered by that file alone, so `protocol.py` holds 98% coverage with the missed-statement count unchanged at 9. The 21 generated fixtures are byte-identical and the generator log is unchanged.
c46f67b to
0a2d79b
Compare
The bullet #640 originally carried, moved here verbatim so that #640 touches only `pcapkit/protocols/protocol.py`, `pcapkit/protocols/application/http.py`, `docs/source/ext.rst`, `examples/generators/dispatch.py` and its six test files. Covers: the behaviour change to a public API -- building a protocol through its constructor with a keyword no signature declares now raises `UnsupportedCall` instead of discarding it. 55 lines added to the entry file; `CHANGELOG.md` regenerated, not edited.
Cross-review of the sentinel change — GOOD TO GOThe sentinel swap was made by an agent, so it went to an independent reviewer on a different model Measured tree, for both of us: What it disputed, and what I changed1. The 2. Two of the four tests pass on the old code — now said plainly in the docstring. True, and by 3. It also called one of my justifications wrong, and it is right. I said reusing What it confirmed, with its own evidence
What it could not verify, stated rather than glossedA Sphinx build with One procedural note, in the interest of it not looking tidier than it was: the reviewer noticed Re-verified after the fixes, on |
The bullet #667 would otherwise have carried, kept here so that #667 touches only `pcapkit/corekit/multidict.py` and `tests/corekit/test_multidict.py`. One bullet, because it is one convention gap in one class: `_Missing` behind `MultiDict.pop` and `OrderedMultiDict.pop` lacked the `@final` and the falsy `__bool__` that `NoValueType` in `pcapkit.corekit.fields.field` sets as the package's convention for a marker of this kind. The bullet says plainly that no behaviour changes, and says why rather than asserting it: both `pop()` implementations decide by identity, never by truthiness, and `pop()` structurally cannot return the marker -- it returns `default` only on the branch where `default is not _missing`. It also names the one way the old truthiness was observable, which is what justifies touching it at all: `inspect.signature(MultiDict.pop).parameters['default'].default` hands the marker to any caller who asks, and `if default:` on it reported "a default was supplied" where none had been. It closes by recording the disposition of the other two sites from the #640 sweep, so the entry is the whole story: site 1 needed nothing, and `_NOT_FOUND` in `pcapkit.utilities.compat` stays a bare `object()` deliberately, being a verbatim line of CPython's `functools.cached_property` inside a `sys.version_info < (3, 8)` branch no supported interpreter reaches. The reasoning behind that one is on #661, not here. `:obj:` roles had to come out: `util/changelog_md.py` rejects them with `ResidualMarkupError`, since its six conversion rules do not cover interpreted text and `CHANGELOG.md` would carry the role through as literal text. Double backticks instead, which is what the rest of the entry file uses. 20 lines added to the entry file; `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited. `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests. Committed from a detached HEAD on e55ba36 and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree at a stale f846523 and could not be taken here. Note e55ba36, not the 69a6e13 I was given: the branch had already moved on with #665's and #651's entries. Refs #661
Fixes #617.
Constructing a protocol with a keyword that no signature declares now raises
pcapkit.utilities.exceptions.UnsupportedCallinstead of silently discarding it.That will surface latent bugs in code that has been quietly losing a field. That is the point of the change, and it is stated here rather than buried: anyone who has a misspelled keyword in a
make/constructor call will get an exception where they previously got wrong octets. It surfaced four such bugs in this repository alone (all fixed here) and three more it deliberately only warns about (below).Parsing is unaffected.
from_datawarns rather than raises.Where the silence was
pcapkit/protocols/protocol.py:284(pre-fix) —Protocol.pack:Every one of the 30
makeimplementations in the tree ends its signature with**kwargs: 'Any'and reads nothing out of it, so a keywordmakedoes not declare is accepted, dropped, and the field it named keeps its default. No warning, no error.The schema layer has never been so permissive —
pcapkit/protocols/schema/schema.py:450, inSchema.__update__, which isSchema.__init__:That asymmetry between the two halves of the same construction is what this closes.
The silent loss, measured
Reproduced against an immutable
git archivesnapshot of6c3d1b0d9in/tmp, with the#602shape ofexamples/generators/options.py'sTCP_BASE—seq,ack_flagandurgent_pointer, none of whichTCP.makedeclares (it spells themseq_no,ack,urgent):Three fields asked for, three fields lost,
warnings: []undersimplefilter('always'). The same construction now:Why a naive fix is wrong, and what was done instead
#617 asked for the design to be decided rather than bolted on, and it was right to. Rejecting on
type(self).make's signature alone breaks three legitimate internal patterns — found by surveying every call site, then confirmed by measurement (54 test failures on the naive version):1.
read-only keywords travel throughmake.ProtocolBase.__post_init__hands onekwargsdict to both halves:So a keyword only
readdeclares still passes throughmake.HIPis the live case from the issue, and it checks out exactly:HIP.read(hip.py:493) declaresextension,HIP.make(hip.py:567) does not, andHIP.__post_init__(hip.py:682) re-forwards it to both — whichexamples/generators/options.py's_hip_builddepends on. Seven protocols do this withextension,AH/ESP/MHalso withversion, andRawwitherror/alias.2.
HTTP.makeis a version dispatcher. It declares onlyversionand forwards everything else toHTTPv1.makeorHTTPv2.makedepending on that value (http.py:150), so no fixed set of names is correct for it.3.
from_dataspreads a machine-generated dict.ProtocolBase.__init__injectspacket=self.packet.payloadinto every parsed_info, and the default_make_dataisdata.to_dict(), sofrom_datahandspacketto amakethat usually does not declare it — starting withNoPayload, reached for the innermost layer of every packet.The design
Checked in
ProtocolBase.__init__— the one point every producer passes through — and only when constructing (file is None). The accepted set is the union of every keyword-taking parameter ofmake,read,pack,unpack,__post_init__and__init__anywhere in the MRO, read frominspect.signatureand cached per class. That is #617's third option, and it acceptsHIP'sextensionwhile still rejectingTCP'sseq.Two escapes for shapes a signature cannot express, both opt-in per class via a new
ProtocolBase.__keywords__:**kwargsby name, asESP.readdoes withkwargs.get('packet'). Unioned down the MRO.None— for a dispatcher that cannot enumerate its own keywords.HTTPis the only user. Not inherited, soHTTPv1/HTTPv2stay checked; inheriting it would have exempted the only two HTTP classes that can be checked, which is what the first attempt did and what a regression test now pins.packetjoins_layer,_protocol,__context__and__packet__as out-of-band, because the library injects it rather than a caller passing it.from_datawarnsUnknownFieldWarninginstead of raising. Its keywords came out of_make_data, not out of anybody's editor, so a mismatch is a disagreement between two of the protocol's own mappings and the caller who meets it cannot fix it.The exception
UnsupportedCall, not a new class. The library already answers "you passed a keyword I do not accept" with it, in this same message shape, twice:ProtocolBase.__init_subclass__(protocol.py:761pre-fix) andDumper.__init_subclass__(dumpkit/common.py:142), both asf'{cls.__name__}: unexpected class keyword(s): {unexpected}'. Recorded deliberately in a test: it carriesAttributeErrorrather than theTypeErrora reader expecting the stdlib's unexpected keyword argument would reach for, and consistency with the two existing precedents was preferred to adding a second answer to one question.Latent bugs surfaced
Four fixed here, all #602 residue — each passing
seq/ack_flag/urgent_pointer, so each was building segments with sequence number0where the mapping read1:examples/generators/dispatch.py_TCP_BASEtests/protocols/transport/test_tcp_mptcp_{length_arithmetic,subtype,capable_length}_unit.py(plus
test_tcp_mptcp_join_flag_ordering_unit.py's one reproduction call, whoseseq=0had to becomeseq_no=0.)Three reported, not fixed —
_make_datareturns a key no signature of the same protocol declares, sofrom_datahas been dropping that field in silence. Each is a defect in its own protocol rather than in this mechanism, and two need a decision about naming rather than a rename:_make_datareturnsmakedeclaresFrame(misc/pcap/frame.py:433)ts_srcts_secL2TPv2(link/l2tpv2.py:367)priopriorityHeader(misc/pcap/header.py:308)magic_numberbyteorder/bigendian/…They are now audible as
UnknownFieldWarning, and recorded as an expected-failure table inReconstructionTests.test_the_three_known_make_data_mismatches_are_recorded, so fixing any one of them turns that test red and the entry gets deleted rather than outliving the defect.Also corrected while here: #617's own note that
tests/test_docstring_contract.py"documents this forwarding pattern as intentional and correct" — flagged there as unverified — is only half right. That module explicitly skips every function with**kwargs(:476-481), calls the exclusion "a known cost … not to be mistaken for coverage" (:73), and asserts nothing aboutmakeat all. It does not pin the swallowing behaviour, and it passes unchanged here (7 passed, 12 subtests).Evidence
A test that fails without the fix and passes with it. New
tests/protocols/test_construction_keyword_check_unit.py, 17 tests / 30 subtests. Against an immutablegit archivesnapshot of6c3d1b0d9(library side hash-verified unmodified,19c398194f73…), with only the test file copied in:and on this branch:
The 5 that pass on both are the no-false-rejection guards — a correct spelling still builds the right octets, a
read-only keyword is still accepted, the parse path still tolerates anything. A rejection test alone would be satisfied by a check that rejects everything.Coverage does not go backwards.
pcapkit/protocols/protocol.py, same scoped test set, before and after:56 statements added, all covered: the missed-statement count is unchanged at 9 and every one is a pre-existing gap, shifted by the insertions. None falls inside an added line range.
EXPECTED_FAILURES: nothing moves.tests/protocols/test_option_roundtrip_unit.py's table cannot be grepped (it is built with**unpacking), so it was imported and dumped, along with the round-trip outcome of every case, on both trees. 45 entries, 322 cases, and the diff of the full table is empty — no entry changes state and none was deleted, including the 6-octetUInt32Fieldpack asymmetry atschema/internet/ipv4.py:368.Generated fixtures are byte-identical.
examples/generators/make_samples.pyruns clean on both trees (exit 0, 21 captures), all 27 files inexamples/captures/hash-identical, and the generator log diff is empty. No generator needed fixing —examples/generators/options.pywas already corrected by #602/#609, which is precisely why the fix lands quietly there.Existing tests whose behaviour changes — two, both in the direction the issue intended:
test_option_generator_tcp_base_unit.py::test_make_still_has_the_kwargs_that_hid_the_defectasserted the silence (assertEqual([...], [])afterTCP(..., no_such_tcp_field=12345)). Its own docstring said a change tomake"turns this red rather than passing silently" — it did, and it is nowtest_an_undeclared_keyword_is_now_refused_rather_than_absorbed, asserting the raise. It keeps the**kwargsassertion, because that is why the check has to live outsidemake.test_protocol_base_unit.py's dummy protocol readvalue/next_protocolout of**kwargsand passedlayer=/protocol=while constructing. Declared as parameters now, which is the pattern this change asks for. That edit passes on both trees, so it is not behaviour-coupled.Full suite green, run in three scoped batches (never all at once — the whole suite in one process hit 41.4 GB RSS on this host):
tests/protocols tests/integration tests/test_docstring_contract.py tests/test_tier_guard.pytests/foundation tests/corekit tests/interface tests/toolkit tests/dumpkittests/cli tests/const tests/project tests/vendor tests/utilities1515 tests, 3392 subtests, zero failures. Exit codes read from a file rather than a shell pipeline.
pcapkit/protocols/protocol.pyhashed before and after the first batch (4b9666affba2…, unchanged) to prove the measurement was not taken against a tree that moved underneath it.python util/changelog_md.py --checkexits 0.Blast radius
pcapkit/protocols/protocol.pyis the most central module in the tree, so, stated plainly:if not parsing:. Dissection keywords —alias,packet, the parse limits — keep flowing as before, because a protocol cannot know which of its ancestors' keywords its parent chose to forward, and nothing was ever lost that way: a dropped parse keyword changes how a packet is read, not what its octets say.protocol.pyishttp.py, which gains__keywords__ = Noneand a comment.pcapkit/protocols/schema/schema.pyis deliberately not changed:Schema.__update__is the constructor of every schema and runs on the parse path too, so tightening it is a separate change with a far wider radius.One commit, on top of
0c7f2b7c9.Review follow-up: the absent-key sentinel is now a typed singleton
Addressing the review comment on
pcapkit/protocols/protocol.py:95— "prefer using an actual classinstance,
None,NotImplementedor custom defined one, likeNoValue" — and the follow-up askingfor a library-wide sweep.
_MISSING = object()is gone:Why a sibling of
NoValueTypeand notNoValueitself, per "maybe not exactlyNoValue":NoValueis documented as "Default value for fields" and asthe value of
FieldBase.default— "was a value given?". This site asks "is this key in the class__dict__at all?". One shared instance would let either site's marker satisfy the other's identitytest, and
NoValuedoes not stay put: it is returned fromSwitchField.pre_process/post_process(
corekit/fields/misc.py:444,:474), written into the packet context(
protocols/schema/schema.py:851), and stored on a schema attribute(
protocols/schema/internet/hopopt.py:709) before being translated back toNoneatprotocols/internet/hopopt.py:1053.protocols/protocol.pyimports nothing fromcorekit/fields/— checked before and after this PR; it takes
context,moduleandprotochainfromcorekitandno field. There is no import cycle either way, so this is preference rather than constraint, but the
protocol base not depending on the field system reads as deliberate.
Why not
None: it is taken at this very site, which is why a sentinel is needed at all.ProtocolBase.__keywords__isOptional[frozenset[str]]andHTTP.__keywords__ = None(
protocols/application/http.py:54) is the opt-out for a dispatcher that cannot enumerate itskeywords, so
keywords is Noneand "the key is absent" are two reachable, different answers. HadNonebeen available the class would not exist.Why not
NotImplemented: its semantics are defined — a binary operator declining an operand —and
bool(NotImplemented)has warned since 3.9 and is slated to raise. Borrowing it would be amisuse and would turn a future
if not keywordsinto a runtime error.Kept module-private.
_Absentis read in_declared_keywordsand discarded there — neverreturned, never cached, never in an error message — so it does not widen the public surface of a PR
already labelled
breaking.Four tests, in
SentinelTests. Two pin the marker's shape: it has a type of its own, it is falsyand
@finalexactly asNoValueTypeis (asserted against that class rather than a literal, so thetwo cannot drift), its
repris<absent>rather than<object object at 0x…>, and it neither isnor compares equal to
NoValue,None,NotImplementedorEllipsis. The other two checkbehaviour instead, because the mistake this kind of swap makes is an identity comparison against the
wrong instance, which reads and typechecks fine. Mutating the default to a fresh
_AbsentType()fails both behaviour tests with
TypeError: '_AbsentType' object is not iterablefromnames.update(keywords), while both shape tests pass through that mutation unharmed — which is whyboth kinds are present.
SentinelTests, pre-change treeSentinelTests, pre-change tree + wrong-instance mutationSentinelTests, this tree+ tests/protocols/test_protocol_base_unit.pyMeasured against
pcapkit.__file__ = …/.claude/worktrees/agent-a40cd115a1e092f53/pcapkit/__init__.py,with every
__editable__*finder stripped fromsys.meta_pathfirst and exit codes read from a file.Nothing else moves.
pylintmessage counts are byte-identical before and after (repo Makefileflags).
mypyreports the same four pre-existing errors with the same codes, line numbers shifted byexactly the six statements added (
742→771,764→793,1408→1437,1751→1780).isort -l100 -ppcapkit --check-onlyis clean.python util/changelog_md.py --checkexits 0. The sentinel's sixstatements are all covered by this test file alone —
coveragereports nothing missed before line442 — so the missed-statement count above is unchanged.
The sweep is #661.
type('...', (), {})()has zero hits package-wide; nothing underpcapkit/const/orpcapkit/vendor/holds an identity sentinel; and with this PR there is exactlyone bare
object()left in the library —_NOT_FOUNDatpcapkit/utilities/compat.py:73, insidethe
sys.version_info < (3, 8)backport of CPython'sfunctools.cached_property, where upstream usesthe same construct for the same reason. It and
_missingincorekit/multidict.py:78-86(already aclass instance, and reachable through two public
pop()signatures and by pickle-by-name) are writtenup in #661 to be decided rather than swept, which is also why they are not in this PR: #640 should not
wait on them. No changelog entry is added here — this is an internal marker with no user-visible
effect, and entries for this cycle go to #657.