corekit: unpack a ListField's schema items from the configured field

[JarryShaw]

Closes #433.

The defect

ListField.unpack's schema branch, pcapkit/corekit/fields/collections.py:166-169:

field = self._item_type(packet)

if is_schema:
    data = cast('SchemaField', self._item_type).unpack(file, packet)

field is the per-item copy SchemaField.__call__ produces: it runs the item's callback on the copy and, if a length_callback was given, resolves the copy's own _length/_template from it (pcapkit/corekit/fields/misc.py:548-566). The schema branch built field and then unpacked from self._item_type instead — the original, never-called object — discarding whatever the copy carried. The sibling non-schema branch a few lines below (length -= field.length, file.read(field.length)) already uses field correctly; only the schema branch had the bug.

As the issue's own follow-up comment established: the callback still runs (line 166 evaluates self._item_type(packet) unconditionally, for its side effect), but its effect on the copy is what gets thrown away, and a length_callback's resolved length never reaches the actual .unpack() call.

Independent census of call sites

Re-derived from git grep -n "item_type=SchemaField" origin/main -- pcapkit, not taken from the issue on trust:

site	item
pcapkit/protocols/schema/internet/hip.py:260	SchemaField(schema=Locator)
pcapkit/protocols/schema/internet/mh.py:535	SchemaField(schema=CGAParameter)
pcapkit/protocols/schema/transport/sctp.py:702	SchemaField(length=4, schema=GapAckBlock)
pcapkit/protocols/schema/transport/tcp.py:393	SchemaField(length=8, schema=SACKBlock)

Same four sites the issue names, and no fifth. None passes callback= or a callable length=, so this confirms the issue's own claim: this is a latent trap, not a live defect. Every in-tree declaration's field copy is equivalent to self._item_type (both resolve to the same static _length, and the default no-op callback), so nothing parses differently today.

The fix

One token: unpack from field instead of from self._item_type.

if is_schema:
    data = cast('SchemaField', field).unpack(file, packet)

cast is kept (only line 404's other usage remains otherwise; import stays needed regardless), because field's static type is the broad FieldBase[Any] and the cast documents that this branch treats it as the SchemaField the is_schema check already established.

pack does not have the mirror-image problem

Checked ListField.pack (same file, lines 90-117) for the equivalent bug. It does not have it, for a structural reason rather than luck: pack never builds a per-item configured copy to discard in the first place. For a schema item it hits isinstance(item, Schema) and calls item.pack(packet) directly (items are Schema instances once unpacked); the elif self._item_type is not None: self._item_type.pack(item, packet) branch is only reached for non-Schema, non-bytes items. Crucially, SchemaField.pack (misc.py:568-595) never consults _length, _template, or anything __call__'s length_callback branch would have set — it just delegates straight to value.pack(...) — so calling _item_type(packet).pack(...) instead of _item_type.pack(...) would be a no-op change even if made. No fix needed there.

Testing

tests/corekit/test_fields_collections.py, new ListFieldSchemaItemTests, two tests, constructing a ListField directly (not through one of the four in-tree declarations, which are exactly the shape that hides the bug):

• test_a_length_callback_is_honoured: an item SchemaField(schema=Item, length=lambda pkt: 2). Item.pre_unpack records packet['__length__'], which is fed by the item field's own resolved .length. Asserts recorded == [2, 2].
• test_a_callback_is_honoured: an item SchemaField(length=1, schema=TypeA, callback=alternate), where alternate mutates field._schema to alternate between two otherwise-identical schemas per item. Asserts seen == ['A', 'B'].

Before the fix (reverted locally to confirm), both fail:

test_a_length_callback_is_honoured: AssertionError: Lists differ: [-1, -1] != [2, 2]
test_a_callback_is_honoured:        AssertionError: Lists differ: ['A', 'A'] != ['A', 'B']

[-1, -1]: self._item_type's raw _length is -1 from __init__ (a callable length= always leaves the raw attribute at -1 until __call__ resolves it), and unpacking from the never-called original never resolves it. ['A', 'A']: the callback's mutation of the copy's _schema never reaches the unconfigured original, so every item unpacks with the schema fixed at construction time.

After the fix, both pass.

Byte-identical capture verification

examples/captures/ is generated (gitignored aside from a handful of committed fixtures) by examples/generators/make_samples.py. Ran it against the buggy tree and the fixed tree and diffed sha256sum over every file in the directory (24 files, including the committed fixtures): identical. Same 18 captures written, same byte sizes, same digests, same "left out"/"warned" lines in both runs.

Full suite

Measured directly, not taken from any other source:

• Baseline, pristine a4c8d62b1 checkout (this branch's actual base; git rev-list --count HEAD..origin/main was 0 at the time this branch was cut): 926 passed, 17 skipped, 1268 subtests passed in 574.78s.
• This branch: 928 passed, 17 skipped, 1268 subtests passed in 587.70s.

The difference is exactly the two new tests; no regressions, no other change in skip/subtest counts.

mypy clean on the changed file. pylint findings on the changed file are unchanged from the origin/main baseline (same list, same lines — none on the changed line).

Note: origin/main advanced to e2d8ed6d1 partway through this session (another PR merged). This branch stays on a4c8d62b1 as the task specified, and both the byte-identical and full-suite comparisons above were measured against that exact sha.

[JarryShaw]

Verified this independently rather than taking the PR's word for it.

Reproduced both sides of the fix. Reverted this one token locally (field → self._item_type) and reran tests/corekit/test_fields_collections.py::ListFieldSchemaItemTests: both new tests fail with exactly the errors quoted in the PR body —

test_a_length_callback_is_honoured: AssertionError: Lists differ: [-1, -1] != [2, 2]
test_a_callback_is_honoured:        AssertionError: Lists differ: ['A', 'A'] != ['A', 'B']

Restored the fix and the whole module passes, 9/9.

Census re-derived, not trusted. git grep -n "item_type=SchemaField" origin/main -- pcapkit finds exactly the four sites named (hip.py:260, mh.py:535, sctp.py:702, tcp.py:393) and no fifth. No SchemaField subclass exists anywhere in the package (git grep -n "SchemaField)" ... | grep -i class is empty), and _item_type is never read or written outside this file (git grep -n "_item_type" origin/main -- pcapkit | grep -v collections.py is empty), so isinstance(self._item_type, SchemaField) can only be true via one of those four literal declarations — no dynamic or indirect route in. None of the four passes callback=, and the length=lambda pkt: … each carries is on the outer ListField, not the inner SchemaField; the SchemaField itself gets either no length= or a static int (length=4 / length=8).

One correction to the PR's own reasoning, not to its conclusion. The PR states that for these four sites __call__ returns a copy "whose _length_callback is None." That's only true for the two sites with an explicit static length= on the inner SchemaField (sctp.py, tcp.py). For the other two (hip.py, mh.py — no length= on the SchemaField), SchemaField.__init__'s own default parameter value for length is lambda _: -1, which is not an int, so _length_callback gets set to that trivial lambda rather than None. Checked directly:

>>> sf = SchemaField(schema=Item)          # no length= passed, like hip.py/mh.py
>>> sf._length_callback is None
False

The practical conclusion still holds — resolving that trivial callback in __call__ reproduces _length=-1, _template='1024s', identical to what __init__ already left it at — so the copy is still equivalent to the original at these two sites. It's "the callback always resolves to the same constant" rather than "the callback is absent" that makes it inert here. Not a code issue, just worth tightening the description.

Byte-identical captures, reproduced independently. Ran examples/generators/make_samples.py at this commit and again with the token reverted, and diffed sha256sum over all 24 files under examples/captures/ (18 generated + 6 committed): identical digests both ways, identical generator log lines.

pack side, confirmed by reading the code. ListField.pack never calls self._item_type(packet) for a schema item — it hits isinstance(item, Schema) → item.pack(packet) directly, since every value produced by the schema branch of unpack is already a Schema instance. SchemaField.pack (misc.py) never references _length, _length_callback, or _template at all. So the elif self._item_type is not None: self._item_type.pack(item, packet) branch is structurally unreachable for SchemaField items in practice, and even if it were reached, .pack doesn't consult the state the bug discards — confirmed both halves independently.

[JarryShaw]

Minor coverage gap, not blocking. This length_callback always returns the constant 2, so both items in the list resolve to the same length. That's sufficient to distinguish field (resolved to 2) from self._item_type (stuck at -1), which is exactly what this test needs to pin the regression this PR fixes — but it doesn't exercise a length_callback whose result varies per item (e.g. keyed off a per-item counter threaded through packet, the way a real item length legitimately could be). A hypothetical implementation that memoized the first resolution and reused it for every later item would still pass this test unchanged.

Confirmed this is a real gap and not just a style question: with the fix reverted, this test already fails on the constant case ([-1, -1] != [2, 2]), so it does catch today's bug. It just wouldn't catch a different bug where resolution happens once instead of per-item. Might be worth a follow-up case with a varying length if this mechanism gets touched again — not requesting a change to this PR for it, since a varying-length case wouldn't add anything to what's being fixed here.

[JarryShaw]

Reviewing in place of Copilot. Head reviewed: b81334ee0d3eae1ece0f391177bf23802af8578c, branch fix-433-listfield-schema-unpack-field. Confirmed refs/pull/453/head fetches to exactly that sha. All code was read with git show <ref>:<path> from that ref or from origin/main, never from an ambient working-tree checkout.

What I checked and actually ran

CI. gh pr view 453 --json statusCheckRollup: 21 SUCCESS, 2 SKIPPED (Docs test gate, Gate (full suite, Python 3.14)), 0 failures — matches the dispatch-time claim exactly.

Position relative to main. origin/main is at e2d8ed6d1. git rev-list --count FETCH_HEAD..origin/main = 1, git rev-list --count origin/main..FETCH_HEAD = 1, merge-base = a4c8d62b1256d84eba2465d1c5ca7c238e3639dd — the branch is exactly 1 commit behind main and 1 commit ahead of their common base, as claimed. git show --stat e2d8ed6d1 (the one commit main gained, PR #435) touches 46 files, none of them pcapkit/corekit/fields/collections.py; git diff a4c8d62b1 origin/main -- pcapkit/corekit/fields/collections.py is empty. No overlap, so the base drift doesn't matter here.

The diff itself. git diff a4c8d62b1 FETCH_HEAD --stat: exactly 2 files, pcapkit/corekit/fields/collections.py (+1/-1) and tests/corekit/test_fields_collections.py (+106/-0, an existing file — confirmed by git show a4c8d62b1:...|wc -l = 318 lines pre-PR vs 424 post). The code change is the literal one-token swap claimed: cast('SchemaField', self._item_type) → cast('SchemaField', field) at collections.py:169.

Census, re-derived rather than trusted. git grep -n "item_type=SchemaField" origin/main -- pcapkit returns exactly the four sites named — hip.py:260, mh.py:535, sctp.py:702, tcp.py:393 — and no fifth. Read all four declarations in full: none passes callback=; the length=lambda pkt: … each carries is on the outer ListField, and the inner SchemaField gets either no length= (hip, mh) or a static int (length=4 sctp, length=8 tcp). Checked for indirect routes too: no SchemaField subclass exists anywhere in the package, and _item_type is never read or written outside collections.py (git grep -n "_item_type" origin/main -- pcapkit | grep -v collections.py is empty) — so isinstance(self._item_type, SchemaField) can only be true via one of those four literal declarations. OptionField (the other ListField subclass) passes item_type=None to super().__init__ and fully overrides unpack itself; it never touches this code path. There's a fifth non-SchemaField item_type= worth flagging for completeness — httpv2.py:285 passes item_type=SettingPair (a raw Schema class, with its own # type: ignore[arg-type]) — but isinstance(SettingPair, SchemaField) is False, so it takes the non-schema else branch, which was never buggy. Unrelated to this fix.

pack, confirmed structurally immune by reading the code. ListField.pack hits isinstance(item, Schema) → item.pack(packet) directly for every schema item (everything the schema branch of unpack returns is already a Schema instance), never calling self._item_type(packet). SchemaField.pack never references _length/_length_callback/_template. Both halves hold.

Tests, reproduced by hand, not just read. Applied the PR's two changed files to my own worktree, ran the module: 9/9 pass. Reverted just the one token (field → self._item_type) and reran the two new tests — both fail with exactly the errors quoted in the PR body:

test_a_length_callback_is_honoured: AssertionError: Lists differ: [-1, -1] != [2, 2]
test_a_callback_is_honoured:        AssertionError: Lists differ: ['A', 'A'] != ['A', 'B']

Restored the fix, both pass again. Left two inline notes on the diff: one is a correction to the PR's own stated mechanism (_length_callback is not actually None for the two sites that pass no explicit length= to the inner SchemaField — it's the default lambda _: -1, which resolves to the same inert result, verified directly with sf._length_callback is None → False) — doesn't change the conclusion. The other flags that test_a_length_callback_is_honoured only exercises a constant length_callback (lambda pkt: 2), not one that varies per item — a real but narrow gap, not blocking.

Byte-identical captures, regenerated myself. examples/captures/ had the 6 committed fixtures and nothing else in a fresh worktree, confirming they're gitignored otherwise. Ran examples/generators/make_samples.py at b81334ee0: 18 files generated, 24 total. Reverted the one token, deleted the 18 generated files, regenerated: same 18 files, same generator log lines. sha256sum over all 24 files, diffed both directions: identical, byte for byte.

mypy / pylint, run myself, not read from the PR. mypy pcapkit/corekit/fields/collections.py: Success: no issues found in 1 source file, both before and after the token revert. pylint on the same file: identical finding list, identical line numbers, identical score 9.29/10 at a4c8d62b1 and at b81334ee0 — none of the findings are on the changed line.

Full suite — measured directly on this branch. PYTHONSAFEPATH=1 PYTHONPATH=<this worktree> .venv/bin/python -m pytest -q at b81334ee0 (pcapkit.__file__ printed and confirmed pointing at this worktree): 928 passed, 17 skipped, 1268 subtests passed in 580.11s — matches the PR's claimed branch figures (it claimed 587.70s; the ~7s difference is normal host variance). Given today's host contention (explicitly flagged to me — full runs have failed to finish in-window for others), I did not also re-run a second ~10-minute full suite at the a4c8d62b1 baseline. Instead: pytest --collect-only -q at a4c8d62b1 collects 943 items (926+17) and at b81334ee0 collects 945 (928+17) — a delta of exactly 2, matching the two new tests — and since the only test-code change between the two revisions is those two tests (confirmed by the diff above) and both pass, 928−2=926 reconstructs the claimed baseline pass count arithmetically rather than by a second direct measurement. Flagging that distinction explicitly rather than presenting it as an independent run.

Verdict

GOOD TO MERGE at b81334ee0d3eae1ece0f391177bf23802af8578c.

The fix is the one-token change it claims to be, the census of affected call sites is exhaustive and accurate (checked for indirect routes, found none), the pack side is confirmed structurally immune by reading SchemaField.pack, the new tests reproduce the claimed before/after failures exactly on revert, the byte-identical-capture claim holds under an independent regeneration, and the branch's own full-suite run reproduces the claimed pass/skip/subtest counts directly. This is a latent-trap fix with no behavioral change today, as described. Left two non-blocking inline notes (a wording correction on the PR's _length_callback reasoning, and a test-coverage gap around varying length_callbacks) for the author's discretion — neither affects this verdict.