fix(corekit): recompute _need_process from the width in force, not once from the placeholder (#591)

[JarryShaw]

The defect

A NumberField whose length is a callable could not pack or parse at any width struct has a native integer code for.

pcapkit/corekit/fields/field.py:541-543 turns a callable length into a placeholder of -1. pcapkit/corekit/fields/numbers.py:109 clears _need_process in __init__, then build_template raises it True in its fall-through branch — which -1 takes, because -1 is not one of {1, 2, 4, 8}. Nothing ever put the flag back, so it was a latch. __call__ resolved the real width and rebuilt the template correctly, the latch survived, pre_process took the process path, self._length was no longer negative so the _length < 0 repair was skipped, and it returned value.to_bytes(...) into a template that was by then >Q.

Parsing was broken in the mirror direction by the same flag: post_process called int.from_bytes on the integer struct.unpack had already produced.

Measured, on fa6d18e31

The comparison is the proof — identical resolved length, identical template, differing only in how the length was supplied:

callable length (resolves to 8):  length=8  _need_process=True   template=>Q  -> error: required argument is not an integer
static length 8:                  length=8  _need_process=False  template=>Q  -> pack ok, 0000010000000000

All four native widths were affected, not only the 8 that #591 reproduces. #591 left this open explicitly; it is established here rather than assumed. The latch has nothing to do with the width it latches into:

=== bare NumberField, callable length ===   (before)
callable -> 1   length=1  _need_process=True   template=>B   pack -> error: required argument is not an integer
callable -> 2   length=2  _need_process=True   template=>H   pack -> error: required argument is not an integer
callable -> 3   length=3  _need_process=True   template=>3s  pack -> 800001            <- correctly unaffected
callable -> 4   length=4  _need_process=True   template=>I   pack -> error: required argument is not an integer
callable -> 8   length=8  _need_process=True   template=>Q   pack -> error: required argument is not an integer

After the fix every callable row matches its static counterpart exactly, width for width, and the 3-octet row is unchanged. EnumField was affected identically, since it also leaves __template__ unset. The eight subclasses that fix __template__ (UInt32Field and friends) never latched anything — __init__ skips build_template entirely for them — and are unchanged.

Placeholder vs. genuine need

build_template takes the same fall-through branch for any width outside {1, 2, 4, 8}, so a callable resolving to 3 legitimately still needs byte packing. Clearing the flag on __call__ would have broken that.

The fix does not track "was a placeholder" at all. It makes the flag a function of the width currently in force rather than a latch: build_template now assigns _need_process instead of only ever raising it. The answer for -1 is True, the answer for 8 is False, and whichever width is in force is the one that decides. The placeholder's True is still correct for the placeholder — it is simply no longer sticky.

One consequence, handled: pre_process now consults the flag after the _length < 0 repair rebuilds the template rather than before it. That repair can itself land on a native width, and deciding first then rebuilding second is precisely how the template and the returned value came to disagree in the first place.

Evidence, verbatim

New tests in tests/corekit/test_fields_numbers_callable_length.py, run against a pristine export of fa6d18e31 and then against this branch.

Before:

FAILED (failures=21, errors=8)
Ran 10 tests in 8.210s

with, among them:

ERROR: test_a_callable_length_roundtrips_through_pack_and_unpack (width=8)
struct.error: required argument is not an integer
FAIL: test_the_reported_case_a_callable_resolving_to_eight_packs
AssertionError: True is not False : the -1 placeholder latched _need_process and #591 is back
FAIL: test_the_flag_always_agrees_with_the_template (width=8, supply='callable')
AssertionError: True is not False : template >Q and _need_process=True disagree

After:

Ran 10 tests in 8.291s

OK

7 of the 10 tests fail without the fix. The 3 that pass either way are deliberate guards against over-correcting: the byte-packed widths (3, 5, 6, 7, 9, 16) must keep _need_process, the unresolved placeholder must keep it, and the __template__ subclasses must be untouched.

Other verification

• coverage run --include='pcapkit/corekit/fields/numbers.py' -m pytest tests/corekit/ — 134 passed, 195 subtests, exit 0.
• tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py — 24 passed, 5 subtests. The 8-octet DSS forms still pack, so fix(tcp): correct MPTCP option length arithmetic at all six #576 sites #585's workaround is undisturbed.
• tests/protocols/test_option_roundtrip_unit.py — 6 passed, 358 subtests. No EXPECTED_FAILURES entry flipped to passing.
• tests/protocols/ tests/corekit/ — 702 passed, 1622 subtests. The 28 failures in that sweep are all missing sample captures in a fresh worktree; after examples/generators/make_samples.py the eight affected files are 32 passed, 7 subtests.
• mypy on the changed module reports no new finding (the one remaining error is pre-existing and present on the unfixed file at the same site).
• python util/changelog_md.py --check exits 0.

Deliberately not done

#585's SwitchField over UInt32Field/UInt64Field in pcapkit/protocols/schema/transport/tcp.py is not redundant now and is left in place. Its NoValueField() branch handles the field being absent from the wire when the DSS flag is clear, which a NumberField(length=<callable>) cannot express whatever this fix does. What has gone stale is only the justification recorded in its docstring — mptcp_dss_ack_selector's note says a corrected lambda "would not have worked" and that fixing it "belongs to pcapkit.corekit.fields.numbers", which is now done. That file is owned by #587 right now, so the prose is left for its owner, as is the same stale paragraph in tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py.

Separately, pre_process's length repair computes math.ceil(value.bit_length() // 8). math.ceil on an int is a no-op, so that is bit_length() // 8, which under-counts: a value of 256 has bit_length() 9 and is sized at 1 octet, and to_bytes(1) then raises OverflowError. That is a distinct defect on a path this PR only makes self-consistent, not one it introduces, and it is left alone rather than folded in.

Fixes #591

[JarryShaw]

✅ GOOD TO MERGE — head 84d8386f85892976766ee424ce8752ad9567731f. One reasoning nuance flagged in the appendix (the SwitchField/NoValueField justification is narrower than the PR states, though its actual scope decision is still correct) — not a blocker.

[JarryShaw]

Cross-review appendix — PR #598

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head 84d8386f85892976766ee424ce8752ad9567731f — this is the PR's current head (confirmed against gh pr list), so no re-pointing is needed despite the rebase that moved #596/#597. Worktree at /tmp/pcapkit-review/pr598, removed after this review.

Fixes keyword and CI

Body ends Fixes #591; closingIssuesReferences = [591]. CI: rollup PENDING, CheckRun tally: 0 FAILURE, 2 SKIPPED (COMPLETED), 21 QUEUED.

The mechanism, independently re-derived from scratch

Read pcapkit/corekit/fields/field.py:278-281 (Field.__init__): a callable length is replaced with the placeholder -1 at construction (self._length_callback, length = length, -1). Read numbers.py's new build_template: every branch now assigns self._need_process (False for 8/4/2/1, True for the fall-through else), rather than the old code which only ever set it True in the else branch and never reset it. Read the reordered pre_process: the _length < 0 repair (which itself calls build_template again, and can therefore flip _need_process back to False) now runs before the if not self._need_process: return value early-return check, not after — so a repair that lands on a native width is reflected in the decision that follows it, rather than being decided on stale information first.

Rather than just running the PR's own test file, I built an independent test harness from scratch:

• Constructed NumberField(length=lambda pkt, w=width: w) for width in {1, 2, 3, 4, 8} on this PR's fixed tree, resolved via field({}), and read _need_process/_template directly: 1/2/4/8 → _need_process=False, matching template (>B/>H/>I/>Q); 3 → _need_process=True, template >3s. Ran the full .pack()/.unpack() round trip through struct for all five widths — every one succeeds, and a static NumberField(length=8) (unaffected, __template__ never involved) behaves identically to the callable-length one for width 8.
• Reverted only pcapkit/corekit/fields/numbers.py to fa6d18e31 and reran the identical harness: widths 1, 2, 4, 8 all raise struct.error: required argument is not an integer on .pack(); width 3 packs fine. This independently confirms the PR's claim that all four native widths are affected, not just the reported 8 — I did not take that on faith, I reproduced it.
• Checked the mirror (parsing) direction the same way: on the reverted tree, .unpack() for widths 1/2/4/8 raises TypeError: cannot convert 'int' object to bytes (because post_process calls int.from_bytes on the int that struct.unpack already produced); width 3 round-trips correctly. On the fixed tree, all five widths round-trip exactly.

Tests fail without the fix, pass with it

Same trap this programme has hit before: an aggregate pytest run on the reverted tree reported 29 failed due to SUBFAILED subtests inflating the count. Ran each of the 10 test methods individually as its own process, reading $?:

test	reverted	fixed
test_a_callable_length_packs_exactly_as_the_same_static_length_does	1	0
test_a_callable_length_roundtrips_through_pack_and_unpack	1	0
test_a_callable_resolving_to_a_byte_packed_width_still_needs_processing	0	0
test_an_unresolved_field_repairs_its_length_and_honours_the_new_template	1	0
test_every_native_width_was_affected_not_only_the_reported_eight	1	0
test_the_flag_always_agrees_with_the_template	1	0
test_the_placeholder_itself_still_needs_processing	0	0
test_the_reported_case_a_callable_resolving_to_eight_packs	1	0
test_a_subclass_fixing_a_template_keeps_working_either_way	0	0
test_an_enum_field_with_a_callable_length_packs	1	0

7 of 10 fail without the fix, exactly as claimed, and the 3 that pass either way map exactly to the PR's three named guard categories (byte-packed width, unresolved placeholder, __template__ subclass) — one test each.

The SwitchField/NoValueField judgment call (as asked)

Read mptcp_dss_ack_selector/mptcp_dss_dsn_selector in pcapkit/protocols/schema/transport/tcp.py and ConditionalField/NoValueField in pcapkit/corekit/fields/misc.py directly, rather than accepting the PR's framing.

The PR's claim, verbatim: "Its NoValueField() branch handles the field being absent from the wire when the DSS flag is clear, which a NumberField(length=<callable>) cannot express whatever this fix does."

Narrowly true, but incomplete. A bare NumberField genuinely has no notion of "don't participate in packing at all" — it always packs some resolved-width bytes. But ConditionalField (already in the same file, misc.py:74-219, not something #598 would need to add) independently supplies exactly that: ConditionalField.pack() is if not self._condition(packet): return b'' and ConditionalField.unpack() is if not self._condition(packet): return self._field.default — it never touches the inner field at all when its own condition is false. So ConditionalField(NumberField(length=lambda pkt: 8 if pkt['flags']['a'] else 4), condition=lambda pkt: pkt['flags']['A']) would, as far as I can tell from reading both classes, now work correctly for presence and width, given #598's fix to NumberField.

This matters because the docstring says the pre-#576 code was exactly ConditionalField wrapping a NumberField(length=lambda pkt: ...) — but that lambda conflated presence and width into one return value (8 if pkt['flags']['a'] else 0, using 0 to mean "absent" rather than using ConditionalField's own condition for that), which is a different and more fragile design than a properly separated presence-lambda/width-lambda pair would be. So the fix that would have made ConditionalField+NumberField viable again isn't really "fix NumberField's callable length" (which #598 does) plus nothing else — it's that, plus decoupling the presence and width lambdas, which is a design change beyond this PR's stated scope.

My judgment: leaving the SwitchField/NoValueField/UInt32Field/UInt64Field structure in place is the right scope decision — it works, it's tested, and swapping it for a ConditionalField-based design would be an unrelated refactor of a file the PR itself says is "owned by #587" (correctly declining to touch it). But the stated reason overclaims slightly: it isn't that NumberField(length=<callable>) categorically cannot participate in an absence-aware design at all — ConditionalField already supplies that half independently. Not a blocking defect — the code and scope decision are both fine — but a place where the prose says more than the code proves, worth recording since it's exactly the kind of claim the review programme wants challenged rather than passed through.

One smaller, non-blocking note: the PR body attributes this structure to "#585's SwitchField," while the in-code comments say mptcp_dss_ack_selector's replacement happened "until #576" and mptcp_dss_dsn_selector's says "C.f. #576" — #585 and #576 both appear to be real, related MPTCP length-arithmetic issues (per PR #597's body, which cites test_tcp_mptcp_length_arithmetic_unit.py as "#576/#585"), so this may be a case of the wrong one being named rather than a fabricated citation, but I didn't chase down which issue actually introduced this specific selector.

Other verification

• python util/changelog_md.py --check exits 0.
• The disclosed-but-not-fixed math.ceil(value.bit_length() // 8) under-counting bug: confirmed directly — (256).bit_length() is 9, 9 // 8 is 1 (math.ceil on an int is a no-op, exactly as the PR notes), and (256).to_bytes(1, 'big') raises OverflowError: int too big to convert. This is a real, separate, pre-existing defect on a path this PR only makes internally consistent; correctly left alone and correctly described.
• tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py + tests/protocols/test_option_roundtrip_unit.py: 30 passed, 363 subtests passed, exit 0 — the fix(tcp): correct MPTCP option length arithmetic at all six #576 sites #585 DSS 8-octet forms still pack correctly, unaffected by this PR.
• coverage run --include='pcapkit/corekit/fields/numbers.py' -m pytest tests/corekit/: 134 passed, 195 subtests passed, exit 0; numbers.py at 96% statement/branch (116 stmts/4 missed, 32 branches/2 partial) — close to, though not an exact re-derivation of, the PR's own coverage claim (I did not chase the precise before/after percentages, treating the pass/fail test evidence above as the load-bearing check).

Not independently checked

• The exact mypy "no new finding" claim was not rerun.
• I did not verify every one of the PR's cited failure messages verbatim (e.g. the exact errors=8/failures=21 unittest tally); the per-test exit-code table above, which I built independently, is the check I'm relying on instead and it reconciles (7 distinct failing tests match their "7 of 10" claim).

Disagreement log

One nuance, not a blocking defect: the SwitchField/NoValueField justification ("a NumberField(length=<callable>) cannot express wire-absence whatever this fix does") is true of NumberField alone but doesn't account for ConditionalField, which already independently supplies absence-handling and could pair with the now-fixed NumberField. The PR's decision not to redesign the schema is still correct; its stated reason for that decision overclaims. Everything else — the core mechanism, all four native widths on both the pack and unpack sides, the 7-of-10 test evidence, the disclosed math.ceil defect, and the regression runs — held up exactly under independent reproduction.