fix(corekit): size the width repair with a real ceiling, not a floored one (#599)

[JarryShaw]

Fixes #599

The defect

NumberField.pre_process derives a width from the value when a field is packed while its length is still the -1 placeholder. It derived it with

self._length = math.ceil(value.bit_length() // 8)

math.ceil of an int is that int — the // has already floored the quotient, so the outer call did nothing and the expression was plain floor division. The width came out one octet short, which both int.to_bytes and struct.pack refuse.

Now math.ceil(value.bit_length() / 8), matching the math.ceil(n / 8) idiom already used in ipv6_route.py:253, hip.py:3156 and pcapng.py:3530. Float division is exact for every input reachable here: bit_length() would have to exceed 2**53 to lose a bit, which is a value of some 10**15 bits.

Re-located line. #598 rewrote this neighbourhood an hour before this change; the line the issue cites as numbers.py:198 is numbers.py:232 in the current tree.

The reach is wider than the report says

#599 frames this as hitting values "just past an octet boundary", which reads as though small values were safe. They are not — floor division is wrong for every bit length that is not an exact multiple of eight. Measured on 13a75dfcd, CPython 3.14.7:

value	bit length	sized at	needs	result
1	1	0	1	OverflowError: int too big to convert
127	7	0	1	OverflowError: int too big to convert
255	8	1	1	ok
256	9	1	2	struct.error: 'B' format requires 0 <= number <= 255
65535	16	2	2	ok
65536	17	2	3	struct.error: 'H' format requires 0 <= number <= 65535
16777215	24	3	3	ok
16777216	25	3	4	OverflowError: int too big to convert

So every value from 1 to 127 failed too, and seven of every eight magnitudes.

Verifying #598's claim, and one correction

#598's author reported this defect and left it, stating that their fix neither fixes nor masks it. That holds — the sizes are byte-for-byte identical across the two versions and every boundary still fails. Reachability is unchanged too: build_template(-1, …) takes the fall-through branch and sets _need_process true in both versions, so the if self._need_process and self._length < 0: guard is entered identically.

But #598 did change what the failure looks like, which the issue's "Measured" block (taken on the pre-#598 fa6d18e31) does not reflect. With _need_process now recomputed from the width in force, a mis-sized 1, 2 or 4 octets is returned as an int and refused by struct.pack; pre-#598 the flag latched true and the same value was refused by int.to_bytes as OverflowError. Measured by restoring the pre-#598 latching build_template:

=== pre-#598 (latching build_template restored) ===
value=255        sized=1 need_process=True -> error: required argument is not an integer
value=256        sized=1 need_process=True -> OverflowError: int too big to convert
value=65536      sized=2 need_process=True -> OverflowError: int too big to convert
value=16777216   sized=3 need_process=True -> OverflowError: int too big to convert

=== post-#598 (current tree) ===
value=255        sized=1 need_process=False -> ok ff
value=256        sized=1 need_process=False -> error: 'B' format requires 0 <= number <= 255
value=65536      sized=2 need_process=False -> error: 'H' format requires 0 <= number <= 65535
value=16777216   sized=3 need_process=True  -> OverflowError: int too big to convert

The OverflowError in the issue title is therefore now only what a mis-sized 3 octets raises. This is why the tests here assert widths and octets rather than exception types.

Reachability, which the issue leaves open

The repair runs only on a field the caller never resolved:

• -1 is assigned in exactly one place — field.py:542, where a non-integer length is swapped for a placeholder and stashed as _length_callback. A field omitting length never becomes an unresolved one; it is refused at construction with the library's own IntError.
• __call__ installs the real width, so a resolved field never satisfies _length < 0. A callable returning a negative, or a static length=-1, does not survive __call__ either — it raises ValueError: negative shift count computing the bit mask first.
• _need_process must also be true, which needs __template__ unset. So it is NumberField and EnumField; the eight Int/UInt subclasses keep _need_process false and never enter the branch.
• Schema.pack resolves every field before packing it (schema.py:655, and :731 for a ConditionalField's inner field), so this is not reachable through a protocol. It is reached through the field-level API — NumberField(length=lambda pkt: 8).pack(value, {}) with no intervening field(packet), which is exactly what fix(corekit): recompute _need_process from the width in force, not once from the placeholder (#591) #598's own test_an_unresolved_field_repairs_its_length_and_honours_the_new_template does.

Which is why the defect survived #591's suite. That test's five repair-path values — 0xFF, 0xFFFF, 0xFFFFFFFF, 0xFFFFFFFFFFFFFFFF, 0x800001 — have bit lengths of 8, 16, 32, 64 and 24. Every one is an exact multiple of eight, precisely where floor division and the ceiling agree.

Tests

New module tests/corekit/test_fields_numbers_width_repair.py, 15 tests. Each boundary gets its own method rather than one parametrised sweep, because the defect is a pattern and a single case would pass against a fix that special-cased the reported width; standalone methods also keep each failure individually visible, since this pytest has no pytest-subtests. Each boundary is asserted as a pair — the value below it, which always packed, and the value above it, which did not — so a width shifted by one in the other direction fails too.

Also: a sweep over all eight boundaries, the ceil(bit_length / 8) invariant, the smallest mis-sized value being 1, a pack/unpack round trip, the root-cause arithmetic (math.ceil(9 // 8) == 1 vs math.ceil(9 / 8) == 2), and controls for the bit lengths that divide by eight and for the reachability of the repair at all.

Proven against the unfixed line — the fix reverted, exit code read from a file rather than the printed summary:

31 failed, 10 passed, 1 warning, 18 subtests passed in 12.12s
=== pytest exit code (read from file): 1 ===

FAILED …::WidthRepairBoundaryTests::test_the_one_octet_boundary_needs_two_octets_past_255
FAILED …::WidthRepairBoundaryTests::test_the_two_octet_boundary_needs_three_octets_past_65535
FAILED …::WidthRepairBoundaryTests::test_the_three_octet_boundary_needs_four_octets_past_16777215
FAILED …::WidthRepairReachabilityTests::test_an_enum_field_is_affected_identically
FAILED …::WidthRepairSurroundingContractTests::test_a_repaired_enum_field_reads_back_a_namespaced_member

and with the fix in place:

15 passed, 1 warning, 44 subtests passed in 11.91s
=== pytest exit code (read from file): 0 ===

13 of the 15 fail without the fix. The 2 that pass either way are the coverage/contract cases — the IntError refusal and the explicit-bit_length branch — and they are what takes the file to full coverage.

Coverage of pcapkit/corekit/fields/numbers.py, scoped with --include per the host-safety rule (never over the whole tree):

baseline (pre-existing numbers suites)  116 stmts  7 miss  32 branch  2 partial   93%   72, 81, 143->147, 507-513
with the new #599 suite                 116 stmts  0 miss  32 branch  0 partial  100%

tests/corekit/ 149 passed, 239 subtests. tests/protocols/test_option_roundtrip_unit.py 6 passed, 358 subtests — no EXPECTED_FAILURES entry flipped, and that file is not touched. No new mypy finding in numbers.py (the one it reports, at :509, is pre-existing in EnumField.post_process and outside this diff).

Found and deliberately not fixed

Three things, all independent of the arithmetic and none made worse here:

1. A signed unresolved field is sized without room for its sign bit. pack(128) on a signed field sizes 1 octet from bit length 8 and is refused with 'b' format requires -128 <= number <= 127 — before and after this change. The ceiling does improve signed sizing (-1 and 127 go from OverflowError to packing correctly), it just does not complete it.
2. An unresolved field's _bit_mask is -1, so the masking and the sign remap at the top of pre_process are both no-ops on this path. That is the underlying reason (1) is incoherent rather than merely off by one.
3. The identical expression survives at two more call sites, both outside this change's files: pcapkit/protocols/internet/hopopt.py:1889 and pcapkit/protocols/internet/ipv6_opts.py:1892, each len=math.ceil(nonce.bit_length() // 8) in the ILNP nonce option builder — compare the correct 4 + math.ceil(random.bit_length() / 8) at hip.py:3156. There is no ILNP entry in EXPECTED_FAILURES, so the roundtrip test is presumably landing on a nonce whose bit length divides by eight, the same blind spot as above. These want their own issue.

Also noted: value = 0 sizes at zero octets and packs to b'', identically before and after. Left alone as a behaviour change beyond this issue.

[JarryShaw]

✅ GOOD TO MERGE — head 5affdf31d0c03bebd74bb39adce99ccd4e5b8740. Reachability, exception-mode reasoning, revert-and-test evidence, and the three disclosed-not-fixed defects all independently re-derived and confirmed; see appendix.

[JarryShaw]

Cross-review appendix — PR #600

Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head 5affdf31d0c03bebd74bb39adce99ccd4e5b8740 in an isolated worktree (/tmp/pcapkit-review/pr600, removed after this review). Per the coordinator's brief, the four specific claims below got the bulk of my budget; I did not re-derive what the coordinator had already verified (the floor-division-is-wrong-for-every-non-multiple-of-8 measurement, or the sibling hopopt.py/ipv6_opts.py #601 sites — confirmed only that #600 correctly leaves those two files untouched).

Fixes keyword and CI

closingIssuesReferences = [599]. CI: rollup PENDING at review time, CheckRun tally 1 SUCCESS, 2 SKIPPED, rest QUEUED/IN_PROGRESS, 0 FAILURE — matches the coordinator's report exactly.

The fix itself, isolated

git diff HEAD~1..HEAD -- pcapkit/corekit/fields/numbers.py (i.e. #600's own commit against 13a75dfcd, the post-#598 base) is a genuinely single-line change: math.ceil(value.bit_length() // 8) → math.ceil(value.bit_length() / 8), plus docstring. Everything else in the file (the _need_process mechanism) is #598's, untouched here.

1. Reachability — traced independently, not accepted

Read the four claims in the code, not the prose:

• pcapkit/corekit/fields/field.py:542: self._length_callback, length = length, -1 — the only place -1 is assigned, and only when length is not an int (a callable). Confirmed via grep.
• NumberField() with no length and no __length__: raises IntError at construction (read in numbers.py's __init__, not independently re-executed since it's a one-line early-raise and not in dispute).
• The branch additionally needs _need_process true, which needs __template__ unset (build_template's fall-through), so only NumberField/EnumField can reach it — the eight Int*Field/UInt*Field subclasses fix __template__ and skip build_template in __init__ entirely.
• The load-bearing claim: pcapkit/protocols/schema/schema.py's Schema.pack() (the file is at pcapkit/protocols/schema/schema.py, not pcapkit/corekit/schema.py as I first guessed) resolves every field via field = field(packet) in its main loop before packing it. I additionally traced the two places a field could dodge that loop's direct resolution and checked both by hand:
  • ConditionalField.__call__ (misc.py) resolves new_self._field = new_self._field(packet) when its condition is true, and ConditionalField.pack()/.unpack() never touch the inner field at all when the condition is false — so the inner field is always either resolved or never invoked.
  • SwitchField.__call__ does new_self._field = new_self._selector(packet)(packet) — it calls the selector and immediately resolves whatever field the selector returns, in one line, before SwitchField.pack() can ever run.
  • Grepped every .pack( call site under pcapkit/protocols/ outside schema.py itself (hopopt.py, ipv6_opts.py, esp.py, tcp.py, hip.py, sctp.py, pcapng.py, httpv2.py, ipv6_route.py, protocol.py) — every one of them calls .pack() on a Schema instance (data.pack(), opt.pack(), param.pack(), etc.), which routes back through the same resolving loop, not on a bare NumberField/EnumField directly.
  • I did not find a counter-example. The "not reachable through a protocol" claim holds under my own tracing, independent of the PR's description of it.

2. Exception-mode reasoning — reproduced exactly

Reverted only the one line (/ → //) on this PR's tree (which already has #598's _need_process fix) and exercised the field-level API directly:

value=255        -> ok len=1 ff
value=256        -> ERROR error: 'B' format requires 0 <= number <= 255
value=65536      -> ERROR error: 'H' format requires 0 <= number <= 65535
value=16777216   -> ERROR OverflowError: int too big to convert

Exact match to the PR's "post-#598 (current tree)" table. Confirms: only the 3-octet crossing (16777216, needing 4, sized at 3 — no native struct code for 3 octets) still raises OverflowError; the 1- and 2-octet crossings now raise struct.error via struct.pack's own range check, because #598 correctly clears _need_process for native widths.

Checked that the tests actually pin width, not luck. Read the assertion bodies directly (not just the docstrings): e.g. test_the_one_octet_boundary_needs_two_octets_past_255 asserts above.pack(256, dict()) == b'\x01\x00' and above._length == 2 — a fix that merely avoided raising (e.g. by wrapping/truncating) would fail the byte-equality check, and a fix that raised for a different but still-wrong reason would fail the _length check. This is checking the correct answer, not merely the absence of an exception.

3. Revert-and-test evidence — reproduced with exit codes read from files

FIXED:    15 passed, 1 warning, 44 subtests passed in 11.87s   (exit 0, read from file)
REVERTED: 31 failed, 10 passed, 1 warning, 18 subtests passed in 12.99s   (exit 1, read from file)

Matches the PR's claimed 15 passed .../44 subtests and 31 failed, 10 passed .../18 subtests almost to the second. Also independently ran all eight boundary values (1, 127, 255, 256, 65535, 65536, 16777215, 16777216) through the fixed field-level API directly — all eight pack at exactly the correct byte length.

Coverage: my first attempt (only the two numbers.py-specific test files) landed at 99%, missing line 103 — a pre-existing, unrelated FieldValueError branch in __init__ from #545 (a signed= contradiction against a fixed __signed__ subclass), not something #600 touches. Rerunning coverage over the PR's actual claimed scope, tests/corekit/ in full: 149 passed, 239 subtests passed, exit 0, pcapkit/corekit/fields/numbers.py at 100% statement and branch (116/116 stmts, 32/32 branches) — exact match to the PR's claim once I used the right scope.

4. The three disclosed-not-fixed defects — each independently reproduced

• Signed sign-bit headroom: NumberField(length=lambda pkt: 8, signed=True).pack(128, {}) raises struct.error: 'b' format requires -128 <= number <= 127 with _length resolved to 1 — confirmed: 128.bit_length() is 8, ceil(8/8) is 1, but a signed 1-octet field's range is [-128, 127], excluding 128. pack(127), pack(-128), pack(-1) all succeed at the same width, confirming the ceiling fix genuinely helps signed values that do fit — it just doesn't add the extra bit signed magnitudes past a power of two need. This is a distinct arithmetic gap (a "+1 for the sign" question, not a floor-vs-ceiling one), correctly out of pre_process sizes a value with floor division disguised as math.ceil, so every octet boundary raises OverflowError #599's stated scope.
• _bit_mask == -1 on a genuinely unresolved field: confirmed directly — NumberField(length=lambda pkt: 8) before any (packet) call has _bit_mask == -1, _bit_length == -1. Traced the consequence: value & -1 == value (no-op mask) and -1 >> 1 == -1, so value > self._bit_mask >> 1 is value > -1, true for any non-negative value, and the "remap" subtracts self._bit_mask + 1 == 0 — a no-op. This is exactly why defect (1) is "incoherent" rather than a clean off-by-one: masking and sign-remapping are silently disabled for a genuinely-unresolved field regardless of the ceiling fix.
• value = 0 packs to b'': confirmed — 0.bit_length() is 0, ceil(0/8) is 0 regardless of / vs //, so this behavior is unaffected by fix(corekit): size the width repair with a real ceiling, not a floored one (#599) #600 either way (it was already this way, and stays this way). Correctly noted as out of scope rather than silently left unmentioned.

Judgment: none of the three should have blocked this PR. All three are pre-existing, independent of the floor/ceiling arithmetic, not worsened by this fix, and honestly disclosed with a reproducible example rather than glossed over. Fixing (1) or (2) properly would need a design decision (how much headroom, whether an unresolved field should even have a usable mask) that's a legitimate separate issue, not a one-line companion to #599's arithmetic fix.

Other verification

python util/changelog_md.py --check exits 0. tests/protocols/test_option_roundtrip_unit.py: 6 passed, 358 subtests passed, exit 0 — no EXPECTED_FAILURES entry flipped, matching the claim.

Not independently checked

• The exact mypy "no new finding" claim (pre-existing finding at numbers.py:509) was read but not rerun through mypy itself.
• I did not chase down whether ILNP nonce builders size with floor division: math.ceil(bit_length() // 8) at hopopt.py:1889 and ipv6_opts.py:1892 #601 (the sibling hopopt.py/ipv6_opts.py sites) has its own issue filed correctly — the coordinator already confirmed those two sites and said not to re-derive it; I only confirmed fix(corekit): size the width repair with a real ceiling, not a floored one (#599) #600 leaves them untouched.

Disagreement log

None. Every claim I was asked to test hardest — reachability, the exception-mode shift, the revert/test evidence, and the three disclosed defects — held up exactly under independent reproduction.