Skip to content

corekit: let a nested schema's field callbacks reach the enclosing schema - #457

Open
JarryShaw wants to merge 10 commits into
mainfrom
fix-445-nested-packet-context
Open

JarryShaw wants to merge 10 commits into
mainfrom
fix-445-nested-packet-context

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 18, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #445.

A nested schema's field callbacks got a packet dict holding the enclosing
schema only under __packet__, so a callback written the ordinary way --
length=lambda pkt: pkt['length'], exactly as every top-level schema writes
it -- raised KeyError the moment the schema was nested. The measured
casualty is CGA Parameters: CGAParameter.extensions
(pcapkit/protocols/schema/internet/mh.py:515-521) sizes itself from
pkt['length'], which belongs to the enclosing CGAParametersOption, and a
well-formed 40-octet option raised KeyError: 'length' in
SchemaField.unpack (pcapkit/corekit/fields/misc.py:619, the
{'__packet__': packet} literal).

Design chosen: chained lookup (option 1 of the three in the issue)

nested_packet_context() (pcapkit/corekit/fields/misc.py) replaces the
literal with a NestedPacketContext, a dict subclass (see "A Python
3.10-only regression" below for why it is a dict subclass rather than
collections.ChainMap, which is where this started). A name the nested
schema does not declare falls through to the enclosing schema; a name it
does declare, or __packet__ itself, is found locally first. This design
was chosen over the other two because:

  • Option 2 (a documented helper) would have required touching every
    callback site, including ones outside this PR's scope, to get the same
    fix mh.py's CGA Parameters needed. The chained lookup needed zero
    changes to mh.py: CGAParameter.extensions's existing
    pkt['length'] just resolves once the mapping falls through. Verified
    by reproducing the issue's exact 40-octet packet before and after.
  • Option 3 (merge the parent in) was rejected for the reason the issue
    names: it lets an inner field silently shadow an outer one. The mapping
    keeps them apart -- see the shadowing test below.
  • The __packet__ contract had exactly one piece of documentation (a
    docstring on an unrelated pcapng.py helper) and zero mentions in
    Schema.unpack's own reserved-key list. Both are fixed: the contract is
    now documented on nested_packet_context itself, and Schema.unpack's
    docstring names __packet__ alongside __length__ and
    __option_padding__.

Write path (explicitly checked, since a mapping that writes through or
drops writes would be worse than today): plain dict assignment and
deletion (pkt[key] = value, del pkt[key]) always act on this instance's
own storage -- that is what a dict subclass gives for free, with no
override needed. So a field a nested schema sets -- including one that
shadows a parent field name -- is never visible to the parent, and never
silently dropped either. in and .get() are overridden (the dict
built-ins for both bypass __missing__ and would otherwise miss the
fallback entirely), and so are __iter__/__len__/keys/values/items,
for the union-of-both-levels iteration the design promises. All of this is
covered by
test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes.

pcapkit/protocols/schema/misc/pcapng.py's two existing __packet__
consumers (packet_byteorder, BlockType.post_process) are untouched.
Both are called directly, in tests and in
pcapkit/foundation/engines/pcapng.py:192, with a plain
{'__packet__': {...}} dict rather than through SchemaField, so their
hand-rolled fallback has to keep handling that shape regardless of this
design -- simplifying them to rely on the chain would have broken those
direct callers. Covered by
test_pcapng_byteorder_consumer_still_works_with_both_shapes and
test_pcapng_block_type_mismatch_consumer_still_works_with_both_shapes,
each exercising both the hand-built dict and NestedPacketContext.

A Python 3.10-only regression, and why NestedPacketContext is a dict subclass

The first pushed version of this PR used collections.ChainMap({'__packet__': packet}, packet) directly, and CI went red on exactly two legs: Python 3.10 and Integration Python 3.10, both on
tests/protocols/misc/test_pcapng_unit.py::PCAPNGUnitTests::test_pcapng_remaining_constructor_branches_and_custom_dispatch,
with AttributeError: 'dict' object has no attribute 'to_dict' from
Schema.to_dict (schema.py:520, isinstance(value, Schema) wrongly true
for a plain dict).

That test has nothing to do with CGA Parameters, HTTP/2, or MH -- it
exercises fifteen unrelated PCAP-NG option constructors. The actual cause,
confirmed with a local Python 3.10 venv:

  • The test passes in isolation on the unmodified tree, and fails in
    isolation
    (no other test file loaded, so none of this PR's new dynamic
    Schema subclasses are even created) as soon as this PR's misc.py
    change alone is applied.
  • Reverting only the collections.ChainMap(...) call back to a plain
    {'__packet__': packet} literal -- nothing else changed -- makes it pass
    again. Restoring the ChainMap call reproduces the failure.

Diagnosis (full version posted on #439): Schema inherits
collections.abc.Mapping (schema.py:245), and on CPython <= 3.10
SchemaMeta.__new__ bypasses ABCMeta.__new__, so no Schema subclass
ever gets its own _abc_impl -- every one of them shares Schema's.
Asking Mapping a question before asking Schema one therefore poisons
the cache for the whole family. collections.ChainMap is itself a
collections.abc.MutableMapping, and constructing one pulled Mapping
into the question order, flipping the unrelated, later
isinstance(some_dict, Schema) check in the PCAP-NG test from False to
True. This is not a fault in the chained-lookup design; it is #439 (not
this PR's to fix) surfacing through an implementation detail of this PR's
own code.

Because the failing test is a plain unittest method, not one of
test_option_roundtrip_unit.py's per-code cases, INTERPRETER_GAPS cannot
express it -- that table only overrides a case.label lookup, and there is
no table to add this to. Rather than leave a real, reproducible CI failure
in place, NestedPacketContext now subclasses dict directly instead of
ChainMap/Mapping. dict's own metaclass is plain type, not
ABCMeta -- constructing or using a dict subclass never asks Mapping
anything, so the shared cache is never touched. Being a real dict also
nominally satisfies every existing packet: 'dict[str, Any]' annotation on
the rest of the field classes, so no other file needed to change (see the
mypy section below for why that mattered).

  • __missing__ gives the enclosing-schema fallback for free: dict's own
    __getitem__ calls it automatically the moment a key is not found
    locally.
  • __contains__ and get are overridden, since the dict built-ins for
    both bypass __missing__ entirely and would otherwise never fall
    through.
  • __iter__/__len__/keys/values/items are overridden for the
    union-of-both-levels iteration the design promises; dict's own versions
    would only see this instance's local keys.
  • copy is overridden because dict.copy() always returns a plain dict,
    even for a subclass, which would silently drop the fallback.
  • Plain assignment and deletion need no override -- dict's own
    __setitem__/__delitem__ already only touch this instance's own
    storage, which is exactly the write isolation the design requires.

Same fallback, same __packet__ reachability, same write isolation as the
ChainMap version -- confirmed by the unchanged test suite (all
pre-existing tests pass unmodified). Verified after the change: the Python
3.10 venv runs tests/protocols/misc/test_pcapng_unit.py clean (37 passed,
139 subtests, 0 failed), and the previously-failing test passes both alone
and as part of that module.

mypy: two errors, from the first NestedPacketContext, now zero net

An intermediate version of this fix (after moving off ChainMap, before
settling on dict) implemented the same two-level mapping from a bare
object deriving from nothing at all. That is a legitimate way to avoid
collections.abc entirely, but it does not satisfy the existing
packet: 'dict[str, Any]' annotation Schema.pack/unpack declare, so
mypy flagged Argument 1 to "pack" of "Schema" has incompatible type "NestedPacketContext"; expected "dict[str, Any] | None", plus an
SchemaField.length type: ignore[has-type] that the restructuring made
newly unused.

Widening Schema.pack/unpack's annotation to admit the new type directly
was the obvious fix and the wrong one: packet flows from there into
pre_unpack/pre_pack/post_process, FieldBase.__call__,
ListField.pack, ConditionalField.test and more, each with its own
narrowly-typed packet: 'dict[str, Any]' signature. Widening only the two
Schema methods produced seven new errors at those call sites; doing it
properly would mean widening every field class's own signature, well
outside this PR's file list.

Making NestedPacketContext a dict subclass (previous section) sidesteps
this too: it satisfies the existing annotation nominally, everywhere,
because it is one. The type: ignore[has-type] was simply deleted rather
than replaced -- and checked: it is already flagged unused on a clean
da2422728 checkout (no changes at all), so it is a pre-existing,
unrelated mypy hygiene gap this restructuring happened to touch, not
something this PR introduced. mypy pcapkit on this PR's head: 123
errors in 40 files
(main: 124) -- one fewer than baseline, and no new
error anywhere.

One stale citation

The httpv2-frame/{DATA,HEADERS,CONTINUATION} Gap entries' defect string
pointed at pcapkit/utilities/decorators.py:222; the actual raise EOFError is at :228 (prepare itself starts at :177). Corrected.

#437's three Exp_FFFD/Exp_FFFE/Exp_FFFF failures

Yes, same defect -- and #437 has since merged (489eef651), which let this
be checked directly rather than by reading its diff. ExperimentalExtension
declares data: 'bytes' = BytesField(length=lambda pkt: pkt['length']), but
length there is CGAExtension's own field (parsed locally, no nesting
problem). The KeyError those three cases hit happened before
ExperimentalExtension was even selected: CGAParameter.extensions's
OptionField has to size the whole extensions area first, via the same
pkt['length'] this PR fixes, regardless of which extension type ends up
inside it. #437 itself registered all three codes and attributed all four
mh-extension/* entries (including Multi_Prefix) to #445 with exactly
that reasoning. See "Second merge" below for what happens to those four
entries once this branch also carries #437 and #446/#456.

CGA Parameters: #446 merged too, so it now parses end to end

This PR alone does not unblock CGA Parameters: with only #445 fixed, it
reaches FieldValueError: Field parameters has invalid length. -- the
separate, already-filed ForwardMatchField/Schema.__len__ defect (#446)
-- instead of the KeyError. That was true when this PR was first opened.
#446 has since merged as #456 (0283a6d59), and with both fixes on the
same tree, the issue's own 40-octet reproduction parses completely: no
exception, a populated CGAParametersOption with one CGAParameter. See
"Second merge" below for the test updates this required.

EXPECTED_FAILURES fallout in the round-trip harness

Fixing the KeyError unblocks the same latent defect in six HTTP/2 frame
schemas (they read the header's flags the same way, on the pack side).
Running tests/protocols/test_option_roundtrip_unit.py after the fix
turned seven cases red against the old table, each hitting a distinct,
unrelated, previously-unreachable defect underneath:

Two more defects surfaced the same way, filed rather than fixed here:
#458 (@prepare raises a bare EOFError for any zero-length schema,
decorators.py:227-228 -- the mechanism behind the DATA/HEADERS/
CONTINUATION entries above) and #459 (above). Both were open when
this was written and have since merged -- see "Third merge" below.

All entries re-attributed with the new status, fragment and file:line,
verified against the actual exception text.

Second merge: main gained #437 and #446/#456 mid-review

main moved again while this PR was in review -- #437 (MH registry
completion) and #456 (issue #446, the ForwardMatchField double-count)
both merged. Merged origin/main (0283a6d59) a second time, one
conflict in the same mh-extension region of EXPECTED_FAILURES
(resolved by re-deriving each entry from the actual post-merge behaviour,
not by picking a side):

Re-verified after this second merge: mypy pcapkit -> 123 errors/40
files
(a fresh origin/main at 0283a6d59: 124, unchanged from the
first merge's baseline). Round-trip harness: 7 passed, 363 subtests
passed
, 0 failed (up from 299 before #437 grew the mh-extension family
to four codes). tests/protocols/internet/test_mh_unit.py: 35 passed, 266
subtests passed, 0 failed. Full suite (Python 3.14,
PYTHONSAFEPATH=1 python -m pytest tests -q): 1001 passed, 17 skipped,
1547 subtests passed, 0 failed
(883s).

NestedPacketContext: from a hand-written mapping to a dict subclass, and back to semantics

The ChainMap-vs-dict diagnosis above turned out to be only half right on
review. #462 merged mid-review and, independently, an owner review thread
asked why nested_packet_context() doesn't just reuse Info
(pcapkit/corekit/infoclass.py) instead of a dedicated class. Rather than
argue from the (now-corrected) ABC-cache mechanism, I built an actual
Info-based nested context on disk and ran it: on a local Python 3.10.21
venv (matching CI's exact patch), tests/protocols/misc/test_pcapng_unit.py
-- the module that broke under ChainMap -- came back clean, 37 passed,
139 subtests, 0 failed. So Info is not unsafe here; the real question is
semantics, not the ABC cache:

  • __missing__-style fall-through: no saving either way. Info.__getitem__
    is a from-scratch self.__dict__[...] lookup with no such hook (that's a
    dict C-level feature), so it needs the same custom code whichever base
    is used.
  • __contains__/.get(): a genuine point for Info -- Mapping supplies
    mixins for both that delegate to __getitem__, so fixing __getitem__
    once gets both for free, where dict's own versions bypass __missing__
    and need separate overrides.
  • Writes landing on the nested instance only: the actual blocker. Info is
    deliberately immutable (__setattr__ raises, no __setitem__ at all),
    but field callbacks write into the packet dict throughout parsing
    (Schema.unpack's per-field loop: packet[field.name] = value, once per
    field). Supporting that on an Info subclass means writing into
    self.__dict__ directly from a custom __setitem__, bypassing rather
    than extending the immutability Info's own docstring promises.
  • Extra bookkeeping to filter: Info instances carry __map__/
    __map_reverse__ in self.__dict__ (for its builtin-name-collision
    handling), which then show up in naive iteration alongside the real
    packet keys -- measured directly, not assumed.

Full reasoning posted on the owner's thread
(#457 (comment));
left unresolved for the owner to close.

Third merge: main gained #461 (closing #458) and #462 (closing #459)

Two more of the newly-surfaced defects this PR had filed (#458, #459) were
fixed and merged while this was in review, as #461 and #462. Merged
origin/main (fa128959e, then e7004191a after reconciling with a
duplicate parallel merge already pushed to this branch) -- no conflicts;
#464's changes to tests/protocols/internet/test_mh_unit.py land in a
different region and were confirmed non-interacting by running that file
(62 passed, 272 subtests, 0 failed).

  • #461 makes @prepare distinguish a declared zero length (a nested
    schema legitimately sized to zero) from a derived one (genuine
    end-of-stream), raising only for the latter. Verified directly:
    httpv2-frame/{DATA,HEADERS,CONTINUATION} all now return 'OK'.
  • #462 wraps SettingsFrame.settings's item type in
    SchemaField(schema=SettingPair). Verified directly:
    httpv2-frame/SETTINGS now returns 'OK' too.

All six httpv2-frame entries this PR's own fix had exposed are now gone:
PUSH_PROMISE/PING closed by #445 itself, DATA/HEADERS/CONTINUATION
by #461, SETTINGS by #462. Rewrote the section comment to summarise all
six rather than describe five stale gaps.

Final numbers, at this branch's current head: mypy pcapkit -> 124
errors/40 files
(a fresh origin/main at fa128959e: 125 -- one more
than its own earlier count, unrelated to this branch, and this branch
stays one fewer than whatever main's own count is, from the same
pre-existing type: ignore cleanup as before). Round-trip harness: 7
passed, 363 subtests, 0 failed. Full suite (Python 3.14): 1010 passed,
17 skipped, 1553 subtests passed, 0 failed
(906s). CI is green: all
22 checks pass (2 skip by design -- the docs gate and the single-version
full-suite gate).

Testing

New file tests/corekit/test_fields_misc_packet_context.py:

  • test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes
    -- the load-bearing case. Before: KeyError: 'length' (matches the
    issue exactly). After: passes, and also checks shadowing, the write
    path, in, .get() and iteration in one pass.
  • test_pcapng_byteorder_consumer_still_works_with_both_shapes,
    test_pcapng_block_type_mismatch_consumer_still_works_with_both_shapes
    -- the two existing consumers, with a hand-built dict and with the new
    chain. Before: ImportError (the helper does not exist yet).
    After: pass.
  • test_cga_parameters_option_reaches_the_446_boundary_not_a_keyerror --
    the issue's own 40-octet reproduction. Before: KeyError: 'length'
    at mh.py:516. After: FieldValueError, confirming the fix worked
    and CGA Parameters correctly still does not parse.

Baseline at e2d8ed6d1 (this branch's original merge-base, confirmed by
temporarily reverting the changed files back to that commit's content in
this worktree, not quoted second-hand): PYTHONSAFEPATH=1 python -m pytest tests -q -> 960 passed, 17 skipped, 1264 subtests passed (617s). After
this PR's original commit, same command: 964 passed, 17 skipped, 1264
subtests passed, 0 failed
(605s).

main then moved four commits (#449, #450, #451, #453) while this PR was in
review, so it was merged (git merge --no-ff origin/main, one clean
auto-merge in the EXPECTED_FAILURES table -- see the heads-up above) and
re-baselined the same way, at the new merge-base da2422728: 977 passed,
17 skipped, 1267 subtests passed
(609s). After this PR's changes on top
(including the NestedPacketContext fix described above): 981 passed, 17
skipped, 1267 subtests passed, 0 failed
(638s) -- the same 4 new tests
join the passing count; the subtest total is unchanged from the new
baseline (main's own commits added tests of their own, which is why 1267
differs from the original 1264).

Also verified on a local Python 3.10 venv (this repo's CI runs 3.10-3.15;
only 3.10 carries #439's risk): 943 passed, 55 skipped, 1237 subtests
passed, 0 failed
-- the higher skip count is only this venv missing some
optional runtime deps (dpkt/pyshark extras), not a difference in outcome.

Test plan

  • New test fails with the exact recorded error before the fix, passes after (verified both directions for all 4 new tests)
  • Both existing pcapng.py __packet__ consumers verified unaffected
  • CGA Parameters reproduction confirmed to reach FieldValueError (ForwardMatchField's non-consuming bytes count toward Schema.__len__, so correct input fails a declared-length check #446), not the KeyError
  • #437's three Exp_FFF* failures confirmed to be this same defect
  • Round-trip harness (tests/protocols/test_option_roundtrip_unit.py) green, EXPECTED_FAILURES updated for all 7 cases whose status changed
  • Full suite green before and after at the stated baseline sha
  • Python 3.10 (isolated test, full test_pcapng_unit.py, and a local full-suite run) confirmed clean after moving off ChainMap
  • mypy pcapkit confirmed at 123 errors/40 files on this PR's head, against 124 on main -- no new error, one pre-existing one incidentally cleaned up

…hema

- SchemaField.pack/unpack handed a nested schema a context of only
  {'__packet__': packet}, so a callback written the ordinary way --
  length=lambda pkt: pkt['length'] -- raised KeyError as soon as a schema
  was nested. Measured casualty: CGA Parameters (mh.py
  CGAParameter.extensions), unparsable via the public API.
- Fix: nested_packet_context() replaces that literal with a two-level
  collections.ChainMap, so a name absent locally falls through to the
  enclosing schema, while __packet__ still reaches it explicitly.
  ChainMap.__setitem__/__delitem__ always act on the nested map, so a
  write never reaches the parent and a shadowed name is read locally.
- mh.py needed no change: CGAParameter.extensions's pkt['length'] now
  resolves via the fallback. CGA Parameters still does not parse -- it
  now reaches FieldValueError: Field parameters has invalid length
  (#446, ForwardMatchField vs. Schema.__len__), which is not fixed here.
- pcapng.py's two __packet__ consumers are untouched: real callers hand
  them a plain {'__packet__': {...}} dict rather than going through
  SchemaField, so their hand-rolled fallback still has to handle that
  shape and is not redundant with this change.
- Documented the __packet__ contract in nested_packet_context, and added
  it to Schema.unpack's reserved-key list alongside __length__ and
  __option_padding__.
- The same latent defect affected six HTTP/2 frame schemas (reading the
  header's 'flags' the same way); updated
  tests/protocols/test_option_roundtrip_unit.py's EXPECTED_FAILURES:
  removed PUSH_PROMISE and PING (now round-trip cleanly), and
  re-attributed Multi_Prefix, DATA, HEADERS, CONTINUATION and SETTINGS to
  the distinct defects this fix newly exposes underneath.

Ran PYTHONSAFEPATH=1 python -m pytest tests -q at baseline e2d8ed6
(960 passed, 17 skipped, 1264 subtests) and after (see PR description).
@JarryShaw
JarryShaw force-pushed the fix-445-nested-packet-context branch from 33c7413 to 46fe59a Compare September 18, 2026 01:12
Comment thread pcapkit/corekit/fields/misc.py Outdated
Comment thread pcapkit/corekit/fields/misc.py
Comment thread tests/protocols/test_option_roundtrip_unit.py Outdated
@JarryShaw

Copy link
Copy Markdown
Owner Author

Reviewing on behalf of Copilot (out of tokens). Head 8a7b4b906, branch fix-445-nested-packet-context. Fetched refs/pull/457/head, confirmed it equals 8a7b4b906. All code read via git show <ref>:<path>, never the ambient working tree; tests run against a real checkout of that exact sha with pcapkit.__file__ printed/asserted before trusting output.

CI

Settled at 19 SUCCESS / 2 FAILURE / 2 SKIPPED:

  • FAILURE: Python 3.10, Integration Python 3.10 — both on the same root cause, detailed below.
  • SUCCESS: Analyze, Compat Python 3.10-3.15 (all 6), Python 3.11-3.15 (all 5), Integration Python 3.11-3.15 (all 5), deploy-pages, CodeQL.
  • SKIPPED: Docs test gate, Gate (full suite, Python 3.14).

git rev-list --count pr457..origin/main = 1 as of this review (origin/main has advanced by one commit, 94a93e721, since the da2422728 snapshot the task was dispatched against — that commit is docs-only, "record the delivery sequence", and doesn't touch pcapkit/, so it doesn't affect this review). At da2422728/dispatch time the PR was 0 behind.

The design: chained lookup via collections.ChainMap

Confirmed by diff (pcapkit/corekit/fields/misc.py): nested_packet_context() returns exactly collections.ChainMap({'__packet__': packet}, packet), used by both SchemaField.pack and SchemaField.unpack in place of the old {'__packet__': packet} literal. mh.py is untouched — confirmed absent from the diff stat (git diff origin/main..pr457 --stat touches only pcapkit/corekit/fields/misc.py, pcapkit/protocols/schema/schema.py, and two test files) — so CGAParameter.extensions's existing pkt['length'] resolves purely through the new fallback, matching the "zero changes to mh.py" claim.

Write path, checked directly rather than taken on faith: ran the new tests/corekit/test_fields_misc_packet_context.py (4 tests) against the PR head — all pass. test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes is the load-bearing one: it exercises __getitem__ fallthrough, the shadowing case (Inner.tag vs Outer.tag staying distinct), in, .get(), iteration (union of both levels), and confirms nothing Inner sets lands in Outer's own packet dict after the fact. This matches ChainMap.__setitem__/__delitem__ semantics (always act on maps[0]) exactly as claimed.

The two pcapng.py __packet__ consumers (packet_byteorder, BlockType.post_process) are confirmed untouched, and the reasoning holds: pcapkit/foundation/engines/pcapng.py:192 really does call with a hand-built plain {'snaplen': ...} dict via the __packet__= protocol-level kwarg (a different mechanism entirely from SchemaField's packet-context dict — that layer is untouched by this PR and out of scope). The new test file's test_pcapng_byteorder_consumer_still_works_with_both_shapes and test_pcapng_block_type_mismatch_consumer_still_works_with_both_shapes both pass, confirming compatibility with what SchemaField now actually builds.

Documentation: Schema.unpack's docstring now names __packet__ alongside __length__/__option_padding__, and nested_packet_context's own docstring documents the contract. Confirmed by diff.

CGA Parameters reaches the #446 boundary, not the old KeyError

Ran test_cga_parameters_option_reaches_the_446_boundary_not_a_keyerror directly against the PR head: passes, and the issue's exact 40-octet reproduction now raises FieldValueError: Field parameters has invalid length (the ForwardMatchField/Schema.__len__ defect, #446, addressed separately in open PR #456) instead of KeyError: 'length'. Confirmed Schema.__len__ and ForwardMatchField are both untouched by this PR (no hits in the diff for either).

HTTP/2 and Multi_Prefix re-attribution

Checked each cited defect against the actual code at 8a7b4b906:

The one real problem: this PR flips a latent #439 case on Python 3.10

Full detail and reproduction is in the inline comment on pcapkit/corekit/fields/misc.py:536. Summary: tests/protocols/misc/test_pcapng_unit.py::PCAPNGUnitTests::test_pcapng_remaining_constructor_branches_and_custom_dispatch fails deterministically on 8a7b4b906 under Python 3.10.20 (isinstance(value, Schema) wrongly returns True for a plain dict, so to_dict() at schema.py:520 calls .to_dict() on it and raises AttributeError), and passes on origin/main under the identical interpreter and test invocation. Reproduced 3/3 times on the PR head, 1/1 on main, both via a driver script that inserts the target tree at sys.path[0] and asserts pcapkit.__file__ before running (the 3.10 venv's own site-packages have no pcapkit installed at all, but the bash tool's cwd was still shadowing plain PYTHONPATH since PYTHONSAFEPATH is a 3.11+-only flag and is silently a no-op on this 3.10.20 interpreter — worth knowing for anyone else testing this repo under 3.10). Also ran the entire Python 3.10 CI job's own command (pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py') locally against 8a7b4b906: 1 failed, 773 passed, 71 skipped, ... 1043 subtests passed — the one failure is this same test, matching CI exactly. The equivalent run under Python 3.14 on this same tree: 840 passed, 5 skipped (0 failures), matching CI's green Python 3.14.

This matches the task's second hypothesis, not the first: INTERPRETER_GAPS is unrelated here (that table only guards test_option_roundtrip_unit.py, and is untouched by this PR's diff), but the mechanism is the same one #439 names — SchemaMeta's shared, order-sensitive ABCMeta cache on CPython <=3.10 — and nested_packet_context introducing collections.ChainMap for the first time against Schema packet dicts is exactly the kind of change that reorders which schema class gets touched first. Since main doesn't have this failure and this PR's head does, it is a real regression this PR introduces, not one it merely inherits. Per the guidance for this situation, it is not this PR's job to fix #439 itself — the right fix is an INTERPRETER_GAPS-equivalent guard on this specific test, citing #439, mirroring the mechanism test_option_roundtrip_unit.py already has. As submitted, Python 3.10 and Integration Python 3.10 are red with no acknowledgement of why.

mypy

Ran mypy against both trees directly: 124 errors in 40 files on origin/main (matches the calibration baseline exactly) vs 125 errors in 40 files on 8a7b4b906. Diffed the two full error lists (normalizing path prefixes) and confirmed the only substantive difference is one new error at pcapkit/corekit/fields/misc.py:640:27 (ChainMap[str, Any] passed where dict[str, Any] | None is expected) — inline comment posted. Every other line-number difference between the two runs is the same pre-existing error shifted by the new function's line count, not a new error.

Verdict

Everything the PR claims about the mechanism, the write path, the pcapng.py scoping decision, the #446 boundary, and the HTTP/2 and Multi_Prefix re-attributions checks out against the code and against test runs I executed myself at 8a7b4b906. The one thing not addressed is real: this PR turns Python 3.10 and Integration Python 3.10 red by flipping a latent #439 case, and ships with no INTERPRETER_GAPS-style acknowledgement of it, plus one small new mypy error and one citation off by six lines.

REQUEST CHANGES — not because the core fix is wrong (it isn't), but because it currently regresses two CI jobs from green to red with nothing in the PR acknowledging why. Add an interpreter-gap-style guard citing #439 for test_pcapng_remaining_constructor_branches_and_custom_dispatch (or otherwise make Python 3.10 green for a stated reason), and optionally clean up the new mypy error, before this merges.

- The previous NestedPacketContext used collections.ChainMap directly.
  ChainMap is itself a collections.abc.MutableMapping, and constructing
  one was enough to disturb the shared _abc_impl cache that every
  Schema subclass uses on CPython <= 3.10 (#439): a later, unrelated
  isinstance(some_dict, Schema) check in
  test_pcapng_remaining_constructor_branches_and_custom_dispatch flipped
  from False to True, raising AttributeError: 'dict' object has no
  attribute 'to_dict' from Schema.to_dict (schema.py:520).
- Measured directly: with everything else unchanged, reverting only the
  ChainMap call back to a plain {'__packet__': packet} literal makes
  that test pass again on Python 3.10; restoring it reproduces the
  failure. Confirmed both in an isolated single-test run and via a
  local Python 3.10 venv (943 passed, 0 failed after this commit,
  against a failure before it).
- Fix: NestedPacketContext is now a plain class implementing
  __getitem__/__setitem__/__delitem__/__contains__/__iter__/__len__/
  get/update/copy/keys/values/items by hand, with none of it deriving
  from collections.abc. Same two-level fallback, same __packet__
  reachability, same write isolation as before -- only the mechanism
  changes, not the contract. All existing tests pass unchanged.
- Not a fix to #439 itself, which remains filed and untouched; this
  only stops this PR's own code from being the thing that trips it.

Verified: PYTHONSAFEPATH=1 pytest tests -q on Python 3.14 (981 passed,
17 skipped, 1267 subtests, 0 failed) and on a local Python 3.10 venv
(943 passed, 55 skipped, 1237 subtests, 0 failed -- the skip count
differs only because that venv lacks some optional runtime deps).
…ation

- NestedPacketContext was a hand-written class implementing the mapping
  protocol from scratch, deriving from nothing -- correct, but it left
  Schema.pack/unpack's "packet: dict[str, Any]" annotation unsatisfied,
  which is what every other field class's pack/unpack still declares.
  Widening those annotations to admit the new type cascades through
  every field class that forwards packet along (ListField, ConditionalField,
  FieldBase.__call__, pre_process/post_process, ...), well outside this
  fix's file list, for seven new mypy errors net.
- Fix: NestedPacketContext now subclasses dict directly. dict's own
  metaclass is plain "type", not ABCMeta, so subclassing or instantiating
  it never touches the shared _abc_impl cache that #439 is about --
  confirmed again on the Python 3.10 venv (this test passes both alone
  and in the full pcapng module: 37 passed, 139 subtests). Being a real
  dict also nominally satisfies every existing "dict[str, Any]"
  annotation, so no other file needs to change.
  - __missing__ gives the enclosing-schema fallback for free, since
    dict.__getitem__ calls it automatically when a key is absent locally.
  - __contains__ and get are overridden because the dict built-ins for
    both bypass __missing__ entirely.
  - __iter__/__len__/keys/values/items are overridden for the union-of-both-
    levels semantics the design promises; dict's own versions would only
    see this instance's local keys.
  - copy is overridden because dict.copy() always returns a plain dict,
    even for a subclass, which would silently drop the fallback.
  - Plain assignment/deletion need no override: dict's own __setitem__/
    __delitem__ already only touch this instance's own storage.
- Removed the now-genuinely-unused "type: ignore[has-type]" this rewrite
  exposed on SchemaField.length -- confirmed present and already-unused on
  a clean origin/main (da24227) checkout too, so this is a pre-existing,
  unrelated mypy hygiene gap this rewrite happened to touch, not something
  introduced by it. Net mypy count: 123 (down from main's own 124), with
  no new errors from this branch's own code.
- Fixed a stale citation: the EOFError-on-zero-length Gap entries pointed
  at decorators.py:222; the actual "raise EOFError" is at :228 (prepare
  itself starts at :177, and #450 shifted lines since the citation was
  written).

Verified: mypy pcapkit -> 123 errors/40 files (main: 124; the diff is the
one pre-existing unused-ignore above, not a new error). pytest tests -q
unaffected on Python 3.14. Local Python 3.10 venv:
test_pcapng_remaining_constructor_branches_and_custom_dispatch passes
alone and as part of the full misc/test_pcapng_unit.py module.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Closing and reopening to re-trigger CI -- the last three pushes to this branch (86370d7, 6ac029a) have not produced any check-runs at all, which looks like a missed webhook delivery rather than anything about the commits themselves.

@JarryShaw JarryShaw closed this Sep 18, 2026
@JarryShaw JarryShaw reopened this Sep 18, 2026
…-context

# Conflicts:
#	tests/protocols/test_option_roundtrip_unit.py
- main merged #437 (MH registry completion, including the four
  mh-extension codes and the _make_ext_multiprefix arithmetic fix) and
  #456/#446 (the ForwardMatchField double-count in Schema.__len__)
  since this branch's last merge. Combined with this PR's own fix,
  all four mh-extension/{Multi_Prefix,Exp_FFFD,Exp_FFFE,Exp_FFFF} cases
  now round-trip cleanly -- verified directly against the round-trip
  harness (all four return 'OK'), not assumed from the PR descriptions.
  Deleted their EXPECTED_FAILURES entries; a stale PARSE/KeyError
  expectation would otherwise have failed this module outright, per its
  own two-way assertion.
- The issue's own 40-octet CGA Parameters reproduction now parses
  completely end to end (confirmed directly: MH(raw, len(raw),
  extension=True) returns a populated CGAParametersOption, no
  exception). Rewrote test_cga_parameters_option_reaches_the_446_boundary
  _not_a_keyerror, which asserted the (now stale) FieldValueError
  boundary, as test_cga_parameters_option_now_parses_end_to_end,
  asserting the parsed fields directly.
- #437 had pinned the pre-fix KeyError as
  test_mh_cga_parameters_option_is_unparsable_upstream, explicitly so
  that "whoever fixes it finds out here" -- and it did: this run turned
  that test red once the merge above landed. Replaced it with
  test_mh_cga_parameters_option_now_parses, asserting the option parses
  and its fields are what the wire says, and fixed the now-stale
  cross-reference and claim in
  test_mh_pmipv6_options_round_trip_byte_for_byte's docstring (CGA_Parameters
  is still excluded from that test's cases, but no longer because it
  cannot be parsed -- that is now a separate, deliberate scope decision
  for whoever adds its full round-trip identity).
- Merged origin/main (0283a6d) with one conflict, in this exact
  region of tests/protocols/test_option_roundtrip_unit.py, resolved by
  re-deriving the correct entries from the actual post-merge behaviour
  rather than picking either side.

Verified: mypy pcapkit -> 123 errors/40 files (a fresh main, 0283a6d,
is 124 -- unchanged from before this merge). Round-trip harness: 7
passed, 363 subtests passed, 0 failed (up from 299 subtests before
#437 grew the mh-extension family to four codes). tests/protocols/
internet/test_mh_unit.py: 35 passed, 266 subtests passed, 0 failed.
Full local suite result to follow in the PR description.
return self._field.unpack(buffer, packet)


class NestedPacketContext(dict):

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about use an Info subclass? and im not sure why must we use a dedicated class for this.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Measured rather than argued from the abc angle (that angle turned out to be a
dead end -- see below), by actually building an Info-based nested context
and running it: it does not break Python 3.10. Swapped NestedPacketContext
for an Info subclass on disk, ran it on a local Python 3.10.21 venv (matching
CI's exact patch), and tests/protocols/misc/test_pcapng_unit.py -- the module
that broke under the ChainMap version -- came back clean: 37 passed, 139
subtests passed, 0 failed, both for the one previously-failing test alone and
for the whole module.

So the earlier "avoids a Mapping tie" reasoning I gave for the dict choice
doesn't hold up, and I'm not using it any more: Info is itself Mapping-based
and gets its own _abc_impl regardless (its metaclass has no CPython-version
bypass), so a Mapping tie alone was never the risk. I'm not citing a
mechanism I haven't personally re-verified, so I'll leave the actual cache
mechanics to whoever measured that -- what I can say directly is that Info
passes the test that mattered.

That still leaves the real question: not "is it safe" but "does it fit". I
built the Info version to answer this honestly rather than guess, and here
is what it would and wouldn't give for free:

  • __missing__ fallback: no saving. Info.__getitem__ is
    self.__dict__[self.__map__.get(name, name)] -- a from-scratch
    implementation with no __missing__-style hook (that's a dict C-level
    feature, not a general Mapping one). Whether I subclass dict or Info,
    I have to write the fallback lookup myself; there's no version where Info
    saves me this code.
  • __contains__/get: a genuine point in Info's favour. Mapping
    supplies mixin implementations of both that delegate to __getitem__, so
    once __getitem__ has the fallback, in and .get() inherit it for free.
    dict's own __contains__/get are C-level and bypass __missing__
    entirely, so my NestedPacketContext(dict) has to override both by hand.
    Info would save that code.
  • Writes must land on the nested instance only, never on the enclosing
    schema
    -- this is where it breaks down. Info is deliberately immutable:
    __setattr__ raises UnsupportedCall, and it has no __setitem__ at all
    (it inherits read-only Mapping, not MutableMapping). Field callbacks
    write into the packet dict constantly during parsing --
    Schema.unpack's own per-field loop does packet[field.name] = value once
    per field, for every field of every nested schema. My Info scratch class
    only supports this by writing into self.__dict__ directly from a custom
    __setitem__, bypassing the immutability Info's own docstring promises
    ("Info objects are immutable, thus cannot set or delete attributes after
    initialisation"). It works, but it's a subclass that quietly defeats the
    guarantee the class exists to provide -- not an extension of Info's
    contract, a contradiction of it.
  • Extra bookkeeping to filter out: Info instances carry __map__ and
    __map_reverse__ in self.__dict__ for its builtin-name-collision
    handling, which then show up in naive iteration alongside the real packet
    keys (I hit this directly -- set(packet.keys()) came back with _parent,
    __map__ and __map_reverse__ mixed in with the actual field names, and
    I'd have needed to filter them the way Info.__iter__ filters
    self.__excluded__). A plain dict subclass has no such baggage: its own
    storage holds only what's explicitly put there.
  • Purpose mismatch, not just mechanics: every real Info subclass in this
    codebase declares a fixed, type-annotated field set and is built once as a
    stable snapshot (that's what info_final/__new__ are for). A nested
    packet context is the opposite: one shared, generic type instantiated fresh
    per parse, holding whatever field names that schema happens to declare,
    mutated field-by-field as parsing proceeds. Reusing the bare Info class
    works because it happens to accept arbitrary kwargs, not because that's
    what it's for.

So: two of the four things this needs (__contains__/get) Info gives for
free; one (__missing__) is a wash; and the write-isolation requirement is a
real contract conflict, not just extra code, because Info advertises
immutability as a feature and a nested context needs exactly the opposite.
That's why I kept the small dedicated class rather than switching -- "least
code that gets the contract right without fighting another class's
guarantees," not "because that's how I already did it." If the immutability
conflict is judged acceptable to paper over (my scratch class shows it's
mechanically possible), I'll switch; I don't think it should be, given the
class's own docstring says the opposite of what the subclass would then do.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still required with #471?

PR #462 merged (bind HTTP.make to a real instance, and wrap SETTINGS'
item schema, closing #459) since this branch's last merge, and pulled
in via the fast-forward to 0e7abbe. SettingsFrame.settings now wraps
its item_type in SchemaField(schema=SettingPair) instead of passing the
raw class, so the AttributeError this entry recorded no longer happens.

Verified directly: tests/protocols/test_option_roundtrip_unit.py's
httpv2-frame/SETTINGS case now returns 'OK'. This is what failed CI on
Python 3.12 and Integration Python 3.15 at 0e7abbe -- not an
interpreter-dependent or order-dependent failure, the same stale-entry
mismatch on every interpreter; those two jobs simply reported first.

Verified: mypy pcapkit -> 123 errors/40 files (unchanged). Round-trip
harness, this file's own tests, and tests/protocols/internet/
test_mh_unit.py: 46 passed, 629 subtests passed, 0 failed.
main gained #461 (closing #458: prepare's @prepare decorator now
distinguishes a declared zero length from a derived one, raising
StreamEOFError only for the latter) since this branch's previous
merge, on top of #462 (closing #459, already handled). Between the
two, all three remaining httpv2-frame entries this PR's own fix had
exposed -- DATA, HEADERS, CONTINUATION -- now round-trip cleanly too.
Verified directly against the round-trip harness: all three return
'OK'. Rewrote the HTTP/2 section's comment block to summarise all six
frames' history (PUSH_PROMISE/PING via #445 itself, DATA/HEADERS/
CONTINUATION via #461, SETTINGS via #462) now that none of them need
an entry.

Also merged origin/main (fa12895, #461) -- clean auto-merge, no
conflicts, confirmed against #464's changes to
tests/protocols/internet/test_mh_unit.py (different hunks, and its own
test run clean: 62 passed, 272 subtests, 0 failed).

Verified: mypy pcapkit -> 124 errors/40 files (main at fa12895: 125,
one more than its previous 124 -- unrelated to this branch, and this
branch stays one fewer than whatever main's own count is, from the
same pre-existing type: ignore cleanup as before). Round-trip harness:
7 passed, 363 subtests passed, 0 failed. mh-extension/* re-verified
'OK' again on this merge (all four).
@JarryShaw

Copy link
Copy Markdown
Owner Author

Reviewing on behalf of Copilot (out of tokens). This supersedes the stale REQUEST CHANGES left at 8a7b4b906 — that review's one real objection (this PR flipped a latent #439 case red on Python 3.10) is fixed on the current head, confirmed below. Head aab958b1d, branch fix-445-nested-packet-context, base main at fa128959e (git rev-list --count pr457..origin/main = 0, still 0 behind). All code read via git show <ref>:<path> or a checkout of that exact sha in an isolated worktree, never a stale working tree; pcapkit.__file__ asserted to start with the worktree root before trusting any test/mypy output.

CI

gh api repos/JarryShaw/PyPCAPKit/commits/aab958b1dd743259e71003a056455507d338c848/status reports state: success. gh pr checks 457: 21 pass, 2 skipping (Docs test gate, Gate (full suite, Python 3.14) — both intentionally gated, not failures), pyup.io/safety-ci pass (this is the StatusContext with .state, not a GitHub Actions run — not evidence of a pending Actions check). Python 3.10 and Integration Python 3.10 — the two jobs the stale review flagged red — both pass (9m1s, 11m19s). That confirms the fix described in the PR body: moving NestedPacketContext off collections.ChainMap and onto a plain dict subclass (commits 86370d7d5, 6ac029a36) stopped disturbing the shared _abc_impl cache from #439.

Full suite and mypy, run myself, not taken from the PR body

Generated fixtures with PYTHONPATH=<worktree> python examples/generators/make_samples.py (required in a fresh worktree; without it ~130 tests raise spurious FileNotFoundError). Then, with the shared repo venv (Python 3.14.7) and pcapkit.__file__ asserted against the worktree root:

  • PYTHONSAFEPATH=1 python -m pytest tests -q1010 passed, 17 skipped, 1553 subtests passed, 0 failed in 840.25s. Exact match to the number claimed for this head. All 4 RUNTIME_DEPS (tbtrim, aenum, chardet, dictdumper) are installed in this venv, which is consistent with the skip count landing at the low end (17) of the stated 17–20 range — noting the correlation, not asserting it as the mechanism.
  • mypy pcapkit on the PR head: 124 errors in 40 files. Swapped only the two touched source files (pcapkit/corekit/fields/misc.py, pcapkit/protocols/schema/schema.py) back to origin/main's content via a /tmp copy (never touched the shared stash) and reran: 125 errors in 40 files. Diffed both full error lists: the only difference is pcapkit/corekit/fields/misc.py:510: error: Unused "type: ignore" comment [unused-ignore], present on main, absent on the PR head. Traced it in the diff: SchemaField.length's getter went from return self._length # type: ignore[has-type] to return self._length — a pre-existing stale ignore comment the PR incidentally dropped while editing that class, unrelated to NestedPacketContext itself. Zero new mypy errors.

The six httpv2-frame and four mh-extension deletions, verified individually, not by trusting the aggregate

Loaded tests/protocols/test_option_roundtrip_unit.py's own harness (OptionRoundTripTests) directly and called self._run(case) on each of the ten cases by name, at the PR head:

httpv2-frame/DATA              status='OK'
httpv2-frame/HEADERS           status='OK'
httpv2-frame/CONTINUATION      status='OK'
httpv2-frame/PUSH_PROMISE      status='OK'
httpv2-frame/PING              status='OK'
httpv2-frame/SETTINGS          status='OK'
mh-extension/Multi_Prefix      status='OK'
mh-extension/Exp_FFFD          status='OK'
mh-extension/Exp_FFFE          status='OK'
mh-extension/Exp_FFFF          status='OK'

Then, to isolate what #445 alone contributes (main has since merged #437/#446/#456/#461/#462, any of which could independently explain a case flipping to 'OK'), I swapped just the two touched files back to origin/main's content and reran the same ten cases against the same harness:

httpv2-frame/DATA              status='CONSTRUCT' detail="KeyError: 'flags'"
httpv2-frame/HEADERS           status='CONSTRUCT' detail="KeyError: 'flags'"
httpv2-frame/CONTINUATION      status='CONSTRUCT' detail="KeyError: 'flags'"
httpv2-frame/PUSH_PROMISE      status='CONSTRUCT' detail="KeyError: 'flags'"
httpv2-frame/PING              status='CONSTRUCT' detail="KeyError: 'flags'"
httpv2-frame/SETTINGS          status='CONSTRUCT' detail="KeyError: 'flags'"
mh-extension/Multi_Prefix      status='PARSE' detail="KeyError: 'length'"
mh-extension/Exp_FFFD          status='PARSE' detail="KeyError: 'length'"
mh-extension/Exp_FFFE          status='PARSE' detail="KeyError: 'length'"
mh-extension/Exp_FFFF          status='PARSE' detail="KeyError: 'length'"

All ten fail, exactly as the deleted EXPECTED_FAILURES entries recorded, with every other merged fix still in the tree. Flipping only misc.py/schema.py is what flips all ten. This holds up: the accounting in the PR body (PUSH_PROMISE/PING clean as soon as #445 landed; DATA/HEADERS/CONTINUATION needing #458#461; SETTINGS needing #459#462; the four mh-extension cases needing #437 and #446/#456 alongside #445) is correct, not just plausible.

Independently re-verified the rest of the accounting too: t.options.cases() returns 322 cases; EXPECTED_FAILURES has 77 entries with zero mh-extension/* and exactly one httpv2-frame/* (PRIORITY, a separate, still-open, unrelated defect — the length = payload + 9 arithmetic — correctly left in place); INTERPRETER_GAPS is untouched at 7 entries. pytest tests/protocols/test_option_roundtrip_unit.py -q → 7 passed, 363 subtests passed.

NestedPacketContext (pcapkit/corekit/fields/misc.py:493), probed directly against the live class

__missing__, __contains__, get, __iter__, __len__, keys/values/items, and copy are all correctly overridden, and I confirmed why copy in particular is load-bearing rather than incidental: Schema.unpack (pcapkit/protocols/schema/schema.py:751, value = field.unpack(byte, packet.copy())) calls .copy() on whatever packet currently is — and for a doubly-nested schema, packet is itself a NestedPacketContext. An unoverridden dict.copy() would silently hand back a plain dict, dropping the fallback for anything nested one level further. The override (misc.py:601-609) avoids that by rebuilding a NestedPacketContext sharing _parent — checked and correct.

Two real gaps in the override set, both reproduced directly against pcapkit.corekit.fields.misc.nested_packet_context:

  • setdefault doesn't consult the fallback. dict.setdefault is C-level and isn't overridden, so it only checks local storage. Given parent = {'length': 40}; ctx = nested_packet_context(parent): 'length' in ctx is True and ctx['length'] is 40, yet ctx.setdefault('length', 999) returns 999 and leaves ctx['length'] == 999 afterward (parent['length'] stays 40, untouched) — it silently shadows a name the mapping itself just reported as present, rather than returning the existing (fallthrough) value the way dict.setdefault is documented to behave for any key already visible on the mapping.
  • pop raises where __getitem__/in/get all succeed. Same setup, fresh context: ctx['length']40, 'length' in ctxTrue, but ctx.pop('length') raises KeyError: 'length'. dict.pop is likewise C-level and unoverridden.
  • Related but lower severity: __eq__ isn't overridden either, so it's dict's own C-level equality — which compares only local storage, not the union __len__/__iter__ present. nested_packet_context({'length': 40, 'other': 'x'}) with ctx['tag']='y' set: len(ctx) == 4 and sorted(ctx) == ['__packet__', 'length', 'other', 'tag'], but ctx == dict(dict.items(ctx)) (i.e., {'__packet__': {...}, 'tag': 'y'}, local storage only) is True. A mapping that presents a 4-key union everywhere else agrees with a 2-key dict under ==.
  • I checked ** unpacking too, expecting the same class of gap (CPython's dict-merge fast path can bypass keys()), but it is actually fine: {**ctx} produces the full 4-key union. NestedPacketContext overriding __iter__ changes its tp_iter slot away from dict's own, which is precisely the condition that makes CPython's dict_merge take the generic keys()/__getitem__ path instead of the direct-hash-table fast path — confirmed by reading the result, not assumed.

I searched the current tree (grep -rn across pcapkit/) for .setdefault(, .pop(, **pkt/**packet, and ==/pkt == on a packet dict, in any field callback, post_process, or condition — there are none. So both setdefault and pop gaps, and the __eq__ inconsistency, are real but currently latent: nothing in this codebase calls them on a packet context today, and none of the shipped tests exercise them. One more, cosmetic: keys() returns a one-shot generator rather than a dict_keys-style view (len(ctx.keys()) raises TypeError, and a second list(...) over the same returned object comes back empty) — acknowledged by the # type: ignore[override] comments already on those methods, and nothing calls len() on it today either.

Write isolation (item 2) and the __packet__ escape hatch (item 3)

Ran the shipped test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes and the two pcapng.py-consumer tests directly (pytest tests/corekit/test_fields_misc_packet_context.py -v): 4/4 pass. Beyond that test, I set a shadowing key directly (ctx2 = nested_packet_context({'length': 40}); ctx2.update({'length': 55}); ctx2['length'] == 55 while parent2['length'] stays 40) — plain dict assignment/update never touches _parent, confirmed, and nothing is silently dropped either.

pcapkit/protocols/schema/misc/pcapng.py's two consumers are untouched by this PR (absent from the diff stat) and stay coherent with the new class: packet_byteorder (:168-169) checks 'byteorder' not in packet and '__packet__' in packet and BlockType.post_process (:354) does packet.get('__packet__', {}) — both only ever reach for __packet__ explicitly, which NestedPacketContext.__init__ always seeds into local storage (super().__init__({'__packet__': packet}), misc.py:558), so both the hand-built {'__packet__': ...} shape and the new class satisfy them identically. I traced the reverse concern too: SchemaField.unpack's nested_packet_context(packet) wraps whatever packet currently is, so for a doubly-nested schema packet['__packet__'] names the immediate enclosing schema, and that schema's own NestedPacketContext (if it is itself nested) carries the chain one level further via its own __missing__ — so a name absent from every intervening level still resolves all the way up, and __packet__ at each level names the schema one level up rather than the topmost one, matching what the docstring promises ("the enclosing schema is also reachable unconditionally").

Also confirmed the two mechanisms sharing the __packet__ name are genuinely distinct and this PR only touches one of them: pcapkit/protocols/protocol.py, ipv4.py, ipv6.py, misc/pcap/frame.py, misc/pcapng.py (protocol-level, not schema-level) and foundation/engines/pcapng.py:192 all use __packet__ as a protocol/Data_*-construction keyword, unrelated to SchemaField's packet-context dict — none of those sites appear in this PR's diff.

The Info-subclass question (comment 4043754294/4043784827) — every checkable claim verified against source

Read pcapkit/corekit/infoclass.py directly rather than taking the reply's characterization on faith:

  • No __missing__ analog: Info.__getitem__ (:332-334) is key = self.__map__.get(name, name); return self.__dict__[key] — a flat lookup with no hook comparable to dict.__missing__. Confirmed as claimed.
  • __contains__/get free via Mapping: Info (:196, class Info(Mapping[str, VT], ...)) defines neither; both come from the Mapping ABC's mixins, which delegate to __getitem__. Confirmed.
  • Immutability is real and structural, not incidental: __setattr__ (:336-337) unconditionally raises UnsupportedCall, there is no __setitem__/__delitem__ anywhere in the file, and the base class list is Mapping, not MutableMapping. Confirmed exactly as described.
  • Schema.unpack's write loop: packet[field.name] = value appears at schema.py:767 (the common case) and the equivalent packet[field.name] = ... appears three more times for PayloadField (:734), PaddingField (:745), and the ConditionalField-false branch (:755). "Once per field" is accurate.
  • __map__/__map_reverse__ leaking into naive iteration: confirmed this is real for exactly the shape the reply describes. The exclusion bookkeeping (cls.__excluded__.extend(cls.__builtin__), adding '__map__', '__map_reverse__', '__builtin__', '__finalised__' to __excluded__) happens only inside the @info_final decorator (infoclass.py:71-77), not automatically for every Info subclass. A bare class Scratch(Info): ... used without that decorator — which is what a generic, instantiated-fresh-per-parse nested context would have to be — has no such filtering, so __map__/__map_reverse__ would show up in set(packet.keys()) exactly as reported. And @info_final itself doesn't fit here anyway: it's built to generate a fixed __init__ from a stable, type-annotated field set (infoclass.py:78+), which is the "purpose mismatch" half of the reply's argument, not just the mechanical half.

Every specific, checkable claim in that reply holds up. The reasoning is sound.

schema.py's docstring addition (+10 lines inside Schema.unpack)

Matches the implementation precisely: "a name this schema does not itself declare falls through to the enclosing schema" (the __missing__ behavior), "the enclosing schema is also reachable unconditionally under a __packet__ key" (seeded unconditionally in __init__) — both accurate, and it correctly scopes itself to when "this schema is nested — unpacked through a SchemaField rather than directly."

One thing this PR's own diff got wrong: a stale ChainMap reference

tests/corekit/test_fields_misc_packet_context.py:24's class docstring still reads: "nested_packet_context replaces that literal with a two-level collections.ChainMap, so a name absent locally falls through to the enclosing schema." That's the design as of 46fe59ad4, before the Python 3.10 regression forced the pivot to a dict subclass in 86370d7d5/6ac029a36. pcapkit/corekit/fields/misc.py's own docstring on NestedPacketContext was correctly updated to explain why ChainMap was rejected (misc.py:538-544), but this test file's docstring, describing the current mechanism, was not updated to match and is simply incorrect about what the code now does. grep -rn "ChainMap" pcapkit/ tests/ shows this is the only stale reference; the two in misc.py are legitimately part of the rejected-design explanation. Doesn't affect what the tests assert or exercise — cosmetic, but worth fixing.

Verdict

CI is green on the actual head (aab958b1d), including the two jobs the superseded review was blocking on. The local full suite (1010 passed, 17 skipped, 1553 subtests, 0 failed) and mypy (124 vs 125, zero new errors, one incidental pre-existing cleanup) both match the PR's own claims exactly, reproduced independently rather than taken on faith. All six httpv2-frame and all four mh-extension EXPECTED_FAILURES deletions are individually verified against the harness's own cases, in both directions — passing at this head, and failing exactly as recorded when isolated back to main's two touched files. NestedPacketContext's override set has two real, currently-latent gaps (setdefault, pop) and one related inconsistency (__eq__), none exercised anywhere in this codebase today; one stale ChainMap reference in the new test file's docstring is a documentation-only leftover. Nothing here contradicts the design, the write-isolation guarantee, the __packet__ escape hatch, or the Info-subclass reasoning given to the owner — all of which check out against the actual source.

GOOD TO MERGE at aab958b1d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A nested schema cannot reach the enclosing packet's fields by name, so CGA Parameters raises KeyError: 'length'

1 participant