fix(corekit): stop a malformed TCP SACK's exception type depending on sys.modules state (#525)

[JarryShaw]

Closes #525.

Which exception a malformed TCP SACK option produced depended on whether the schema layer had already been exercised in the same process — ProtocolError in a clean process, FieldValueError from pcapkit/corekit/fields/collections.py after tests/protocols/schema/ had run. The two are siblings in the exception hierarchy, so no single except clause caught both, and which one you got was not a property of the input. A consumer writing except ProtocolError around Extractor got code that worked in tests and short scripts and stopped working in a long-lived process.

The issue recorded the mechanism as never established. It is established now, and it is not caching or lazy initialisation as the issue guessed:

ListField.unpack resolved SchemaField through a function-local import, re-run on every call instead of once at module load. That is a cheap sys.modules hit almost always — except when something has popped pcapkit.corekit.fields.misc out of sys.modules since that ListField was built, which is exactly what #439's ABC-cache regression tests do in every case's setUp/tearDown to get a clean cache. The next call re-executes the module and mints a second, distinct SchemaField class, while the SACK sack field's item_type — built long before from the first one — is still an instance of the original. isinstance against the new class answers False for an item that plainly is a SchemaField, so ListField.unpack takes its other branch, which bills each item by its declared length unconditionally rather than by what it consumed. For a malformed SACK the length accounting goes negative and raises FieldValueError before TCP._read_mode_sack is ever reached.

So the state dependence was never about SACK at all — it was an import-time identity bug reachable by any ListField whose module got evicted.

• Hoist the SchemaField import to module level in collections.py.
• Narrow tests/protocols/transport/test_tcp_sack_length_unit.py's union assertion to the single correct type, now that the behaviour is deterministic.

length == 2 remains accepted: (2 - 2) % 8 == 0 satisfies the documented rule exactly. RFC 2018 also requires at least one block, so a stricter check is arguably warranted, but tightening past the docstring was deliberately left alone.

4 files, +148/-39, one commit, on top of 8cfd6ab01.

Verification (replaces the earlier "not verified" note — CI was failing on this branch for a separate, now-fixed reason): CI was red on 13 checks, all traced to one cause named by the Changelog drift check: the committed CHANGELOG.md had been hand-edited rather than generated, and differed from util/changelog_md.py's output only in where the bold markers sit around the FieldValueError/ProtocolError literals in the SACK entry — docs/source/changelog/1.5.0.rst already has the bold correctly split around each literal (reStructuredText has no way to span inline markup across a literal), and the hand-written CHANGELOG.md had merged it into one run. Fixed by regenerating: python util/changelog_md.py, confirmed with python util/changelog_md.py --check (exit 0, "is in step with"). This branch also needed rebasing onto current main to flatten a Merge branch 'main' commit the owner had added; the rebase replayed cleanly with no conflicts, and I confirmed by hand that both PR #560's EnumSchema.registry changelog entry and this PR's SACK entry are present in the rebased CHANGELOG.md and 1.5.0.rst. tests/project/test_changelog_md.py: 34 passed, 37 subtests passed (exit 0). Full unit tier, pytest tests (no coverage, per host memory limits): first run showed 1 failure in tests/protocols/test_option_coverage_runtime.py::OptionCoverageCaptureTests::test_option_captures_are_what_the_generator_says_they_are — traced to a stale, gitignored local fixture (examples/captures/options-ipv4.pcap) generated before PR #559's IPv4 TS option round-trip fix landed on main; CI regenerates this fixture fresh via examples/generators/make_samples.py on every run (.github/workflows/unit-tests.yml), so it never sees this staleness, and confirmed the fix by regenerating the fixture locally (examples/generators/make_samples.py, exit 0) and re-running: 1261 passed, 17 skipped, 2850 subtests passed (exit 0). All exit codes read from files, not from summary lines. Amended into the single commit and force-pushed.

One open question for the reviewer, unchanged from before: the fix lands entirely in collections.py and does not touch pcapkit/protocols/transport/tcp.py, which follows from the mechanism above but is worth confirming, along with whether any docstring still promises the exception that no longer escapes.

[JarryShaw]

✅ GOOD TO MERGE

Cross-model review (Opus 5; the PR was authored on Sonnet), head 86f56197e.

I reproduced the entire mechanism independently, and every element of it holds. On main, popping pcapkit.corekit.fields.misc from sys.modules flips the exception from ProtocolError to FieldValueError; on this branch it does not. The narrative in the description is not a plausible story fitted to a symptom — it is the actual cause, and I verified each link in it separately: the function-local import re-runs, the module is re-executed, a second distinct SchemaField class is minted, and the real SACK item_type is an instance of the first but not the second.

Both of your open questions, answered:

• The fix belongs in collections.py and correctly does not touch tcp.py. Confirmed: _read_mode_sack's check is already right, and the fault was that FieldValueError escaped from collections.py:193 before _read_mode_sack ran at all. Nothing in tcp.py needed changing.
• No docstring still promises the exception that no longer escapes. ListField.unpack's Raises: FieldValueError is still accurate — a genuine overrun can still raise it — and _read_mode_sack's Raises: ProtocolError is now the truth rather than a half-truth. I grepped the tree for FieldValueError and every remaining mention is ipaddress.py and its tests, unrelated to SACK.

I also independently confirmed the two things I was asked to be sceptical about: the hoist introduces no import cycle (fields/misc.py imports only fields.field and utilities.exceptions, and never references collections), and length == 2 is still accepted — (2-2) % 8 == 0, and test_documented_rule_is_the_implemented_rule still asserts parse(sack_option(2, 0)) succeeds, untouched by this diff.

Your full-suite numbers reproduce exactly: 1261 passed, 17 skipped, 2850 subtests passed, exit code 0 (read from a file, 19:59). Changelog drift check exits 0. The test genuinely fails without the fix — I measured it.

Three follow-up notes, none blocking, in the detailed comment below. The most useful one: the same identity pattern survives at collections.py:105 in ListField.pack, where it cannot be hoisted because schema.py imports collections.py for real. I demonstrated that a second Schema class does get minted there and isinstance does answer False for a genuine schema — it just happens to be masked in the SACK path. Worth its own issue.

[JarryShaw]

Detailed cross-review — #562 @ 86f56197e

Reviewer: Opus 5, per the standing rule that an agent-raised PR gets a cross-review from a different model than the one that wrote it. All measurements on CPython 3.14.7, with PYTHONSAFEPATH=1 and PYTHONPATH pinned to my worktree, asserting pcapkit.__file__ resolves inside it before importing anything else. Exit codes read from files, never from summary lines.

Shape: one commit on top of current main 8cfd6ab01, no merge commit. 4 files, +148/−39.

---

1. The mechanism — reproduced end to end, on both trees

I wrote a standalone probe rather than trusting the test: build the same malformed SACK the test uses (declared length=11), parse it in a clean process, then pop pcapkit.corekit.fields.misc from sys.modules exactly as purge_modules does, and parse again.

On main @ 8cfd6ab01:

clean:        pcapkit.utilities.exceptions.ProtocolError:   TCP: [OptNo 5] invalid format
after purge:  pcapkit.utilities.exceptions.FieldValueError: Field sack has invalid length.
misc re-imported by the parse?          True
SchemaField v1 is v2?                   False
-> a SECOND distinct SchemaField class was minted
deterministic?                          False

On this branch @ 86f56197e:

clean:        pcapkit.utilities.exceptions.ProtocolError: TCP: [OptNo 5] invalid format
after purge:  pcapkit.utilities.exceptions.ProtocolError: TCP: [OptNo 5] invalid format
misc re-imported by the parse?          False
deterministic?                          True

That is the whole of #525, on and off. Note the middle line in each block: on main the parse itself puts the module back, which is the function-local import firing; on this branch it does not, which is the module-level import holding.

And the isinstance failure at the heart of it, measured directly against the real field rather than a synthetic one:

the real SACK item_type is a SchemaField from pcapkit.corekit.fields.misc
isinstance(real_item_type, ORIGINAL SchemaField) = True
isinstance(real_item_type, FRESH    SchemaField) = False

So is_schema really does go the wrong way for an object that plainly is a SchemaField. Every link in the description's chain is independently confirmed.

The "siblings, so no single except caught both" claim is also exact. Both ProtocolError and FieldValueError derive from BaseError → ValueError; issubclass is False in both directions. A consumer could only have caught both by widening to BaseError or ValueError.

2. The #439 attribution is correct — but findable only via purge_modules

Worth flagging because it cost me a detour and will cost the next reader one. The comment and docstrings say the #439 ABC-cache regression tests "pop pcapkit.corekit.fields.misc out of sys.modules" in every setUp/tearDown. Grepping tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py for sys.modules returns nothing, and I briefly took the attribution to be invented.

It is not. The eviction is real, one level down:

# tests/_support.py:271
def purge_modules(prefixes: 'Iterable[str]') -> None:
    for name in list(sys.modules):
        if any(name == prefix or name.startswith(prefix + '.') for prefix in prefixes):
            sys.modules.pop(name, None)
    _reset_abc_caches()

and that file's setUp/tearDown call purge_modules(['pcapkit']), which matches pcapkit.corekit.fields.misc on the prefix rule. Suggestion: name purge_modules (or tests/_support.py:271) in the collections.py comment. The claim as written sends a reader to a file where the evidence is not visible.

3. No import cycle — verified, and the scoping is precise

This was my main correctness worry about hoisting. pcapkit/corekit/fields/misc.py's only module-level pcapkit imports are fields.field and utilities.exceptions; it never references fields.collections, at module level or inside any function. The comment's claim is accurate.

The scoping is also carefully right, which I want to credit explicitly: the comment says "pcapkit.corekit.fields.misc does not import this module" rather than a general claim about in-function imports — and that limitation is load-bearing, because the sibling import two methods above is cycle-bound (see §6).

4. Fails-without-the-fix — measured

Put this branch's test file on main's source and ran it:

tree	exit code (from file)	result
main source + this PR's test file	1	FAILED …::test_sack_exception_type_survives_a_sys_modules_purge
this branch, whole file	0	4 passed, 10 subtests

and on main it fails for exactly the right reason, not incidentally:

E   pcapkit.utilities.exceptions.FieldValueError: Field sack has invalid length.
pcapkit/corekit/fields/collections.py:193: FieldValueError

On the collection-order question: the new test does not need a subprocess, and I think that is the right call. It performs the sys.modules manipulation itself rather than depending on another suite having run first, so it fails on broken code in an ordinary in-process pytest run — measured above. It also cannot pass on broken code by ordering luck, because assertNotIn('pcapkit.corekit.fields.misc', sys.modules) can only hold when the import is resolved at module level; on main the parse demonstrably puts the module back.

Note that the narrowing of test_invalid_sack_length_is_rejected is not itself a fails-without test — it passes on main too, since a clean pytest process already produces ProtocolError. The new purge test is what carries the regression coverage. That follows from the mechanism and is not a criticism, but it is worth being clear that the union-narrowing alone would not have caught a regression.

5. length == 2, and the deliberate non-tightening

Still accepted, and still pinned. The implementation is if (schema.length - 2) % 8 != 0: raise, so length=2 satisfies it; test_documented_rule_is_the_implemented_rule asserts parse(sack_option(2, 0)) succeeds and that a five-block SACK cannot even be expressed (data offset is 4 bits → options area ≤ 40 octets). That test is untouched by this diff. Tightening past the docstring was correctly left alone.

6. Follow-up: the same identity pattern survives in ListField.pack

ListField.pack — two methods above the one fixed — still resolves Schema through a function-local import:

# pcapkit/corekit/fields/collections.py:105
from pcapkit.protocols.schema.schema import \
    Schema  # pylint: disable=import-outside-top-level
...
elif isinstance(item, Schema):

This one cannot be hoisted — pcapkit/protocols/schema/schema.py:11 does from pcapkit.corekit.fields.collections import ListField, OptionField, so a module-level import here is a genuine cycle. So it is not an oversight in this PR, and I am not asking for it here.

But the hazard is real and I demonstrated it. Popping pcapkit.protocols.schema.schema and re-packing a SACK schema:

schema module re-imported by the pack?              True
Schema v1 is v2?                                    False
-> second distinct Schema class minted:             True
isinstance(a real SACKBlock, ORIGINAL Schema) =     True
isinstance(a real SACKBlock, FRESH    Schema) =     False

The isinstance does misclassify a genuine schema. In this path the outcome is benign — the bytes are identical (050a0000000100000002 both times) because self._item_type is not None for SACK, so the next branch packs the item the same way. The masking is incidental, though: for a ListField with _item_type is None carrying Schema items, the misclassification falls through to

raise FieldValueError(f'Field {self.name} has invalid value.')

which is the #525 shape exactly. I did not establish whether any such field exists in the library today, so I am not claiming a live bug — but the description's own framing ("an import-time identity bug reachable by any ListField whose module got evicted") raises the question and does not answer it. Worth an issue, with the cycle noted so nobody tries to fix it by hoisting.

7. Changelog

Generated, not hand-matched — verified with the repository's own check, which is what CI runs as Changelog drift:

$ python util/changelog_md.py --check
CHANGELOG.md is in step with docs/source/changelog/1.5.0.rst
exit 0

The entry itself is accurate against everything I measured, including the behaviour-change warning that a caller relying on FieldValueError now gets ProtocolError. That warning is correct and is the right thing to have flagged.

8. Full suite

Ran pytest tests (no coverage, per the host memory constraint — a coverage run -m pytest tests from a sibling worktree reached 36.9 GB RSS earlier and exhausted this machine). This worktree was fresh, so examples/generators/make_samples.py ran first (exit 0).

1261 passed, 17 skipped, 11167 warnings, 2850 subtests passed in 1199.43s (0:19:59)
pytest exit code: 0

That matches the description's claimed numbers exactly — 1261 / 17 / 2850 / exit 0. The provenance caveat about unverified test results can be dropped; the numbers are now independently reproduced.

9. Minor, non-blocking

The new test opens with

self.assertIn('pcapkit.corekit.fields.misc', sys.modules,
              'test setup expects pcapkit to already be imported')

which asserts a precondition about global state the test does not itself establish — and several suites in this repository purge pcapkit.* from sys.modules (test_schema_metaclass_abc_cache_unit.py via purge_modules, and tests/cli/test_main.py directly). I could not make it fail: it passes with the whole ABC-cache module running first (exit 0), and with only the tearDown-bearing class running immediately before (exit 0), and in the full suite. So this is a robustness suggestion, not a finding — importlib.import_module('pcapkit.corekit.fields.misc') would make the precondition true by construction rather than by whatever happened to run first, and would avoid a future confusing failure that has nothing to do with SACK.

Could not verify

• CI. Reached deliberately on local evidence only; I did not wait on GitHub Actions and make no claim about its tally.
• Any interpreter other than CPython 3.14.7. Single-version throughout.
• Whether a ListField with _item_type is None and Schema items exists today (see §6) — I did not enumerate every ListField construction in the library.
• The original Malformed TCP SACK raises ProtocolError or FieldValueError depending on process state #525 report's trigger. I reproduced the mechanism by popping the module by hand; I did not confirm which specific historical suite ordering produced the originally-observed FieldValueError. The description doesn't claim to either, and given purge_modules is called from setUp/tearDown the route is clear enough.