Skip to content

fix(corekit): keep a NumberField's bits outside bit_length as captured (#1488) - #1497

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1488-numberfield-bit-length
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1488-numberfield-bit-length

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

bit_length says only the low N bits of a field's octets are its value (vlan.TCI, which no protocol parses with). Unpack and pack both masked, so the other bits were lost.

  • post_process still returns the low N bits. If the other bits differ from what pre_process writes for that value (zeros, or sign extension for a negative signed value), it returns _CapturedInt, an int subclass that records (outside, length, bit_length, signed, byteorder).
  • pre_process writes the bits back only when its own length, bit_length, sign and byte order all match the record. Any other field packs the value exactly as pack(int(v)) would, e.g. UInt16Field() gives 0fff and UInt8Field() gives ff.
  • EnumField puts the same record on the pseudo-member, or on an unregistered copy of a registry member (_unregistered_member). The copy is still isinstance and equal, but is no longer the identical object. One assertIs pin in test_fields_numbers_unassigned_enum.py is relaxed to match.
  • A value you supply, or get by arithmetic on a parsed one, is still truncated to N bits as before (0x1fff gives 0fff).
  • The Gap(1488) row is deleted.

Ran (sequentially): new test, tests/corekit, tests/protocols/link, tests/protocols/test_capture_roundtrip_runtime.py, tests/project, tests/test_docstring_contract.py, all green. The new test fails on main's numbers.py, and its scoping cases fail on the previous head 687596c53. repr(frame.info.to_dict()) hashes over all 23 examples/captures match main.

Closes #1488

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 687596c53. Cross-reviewed on Sonnet; the author ran on Opus. The design is acceptable; one hardening is required.

Required: a captured value must carry its capture conditions, and only a field with the same width and bit_length may honour them. Today the outside bits are stored bare, so the value leaks into other fields. I re-derived this myself at head:

Field pack(v) where v = UInt16Field(bit_length=12).unpack(b'\xff\xff') pack(4095)
UInt16Field() ffff 0fff
UInt16Field(bit_length=8) f0ff 00ff
UInt8Field() struct.error —

v == 4095 holds throughout. Fix: store (outside, length, bit_length) with the value, and have pack ignore the capture when they differ. The same applies to the enum copies. The leak isn't reachable from a parse today (vlan reads TCI through a BitField), but from_dict or make would reach it.

What checked out:

  • All 65,536 two-byte inputs round-trip exactly, unsigned, signed and little-endian, and arithmetic returns a plain int.
  • No is <Enum>. comparison in pcapkit/ sees these values.
  • The per-capture to_dict() hashes are identical across all 23 captures.

Also needed: the Gap(1488…) row now conflicts with main (00aa1fc90), because #1494 deleted the adjacent row. Rebase.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.65% (unit tier, Python 3.14, 687596c53, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 958 2356 871 90.99%
pcapkit/corekit 2136 68 684 29 96.21%
pcapkit/dumpkit 242 2 88 2 98.79%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 17086 238 4628 190 97.98%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw force-pushed the fix/1488-numberfield-bit-length branch from 687596c to 656dc4c Compare October 9, 2026 20:20
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 656dc4c9f. This is the round-2 cross-review, run on Sonnet; the author ran on Opus. Rebased onto 00aa1fc90.

  • The round-1 leak is fixed. A capture now records (outside, length, bit_length, signed, byteorder) and is honoured only by a field of the same shape. Re-derived by me: same shape packs ffff, while UInt16Field(), UInt16Field(bit_length=8) and UInt8Field() all equal pack(int(v)) (0fff, 00ff, ff).
  • Through Schema.from_dict: a captured 4095 packs ffff into the same shape. Into bit_length=8, wide and little-endian targets it packs 00ff, 0fff and ff0f. Copies, deepcopy and pickle keep the record.
  • Arithmetic and enums: arithmetic, int(), hash and repr behave as for a plain int. The registry is untouched: E(1) is E.A still holds, and an unpack with no spare bits still returns the member itself.
  • Diff scope: test_field_roundtrip_unit.py changes only by removing the Gap(1488…) row. The relaxed assertIs is justified, and the plain case keeps identity.
  • Regression runs: the per-capture to_dict() hashes are identical across all 23 captures. tests/corekit 493 passed; link plus capture round trip 93 passed; tests/project 419 passed.

Non-blocking:

  • The capture is keyed on shape, not enum namespace, so a same-shape EnumField with a different namespace honours it.
  • Pickling a pseudo-member fails as it already does on main.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1488-numberfield-bit-length branch from 656dc4c to 1c626ed Compare October 9, 2026 20:39
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1488-numberfield-bit-length branch from 1c626ed to 8da3cbd Compare October 9, 2026 20:40
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict carried to 8da3cbd00: GOOD TO GO. I rebased this onto main 934c343b5, resolving one conflict, after #1495 deleted the Gap(1487…) row beside this PR's Gap(1488…) row.

  • Library change unchanged: the stable patch-id of the pcapkit/ diff is a8244c3d21a4 before and after.
  • Test-file resolution, by me: in tests/corekit/test_field_roundtrip_unit.py both rows are now gone, so the table reads KNOWN_FAILURES = (), and the now-unused Gap import is dropped. Nothing else in the file changed.
  • Re-run at the new head: the new test, test_field_roundtrip_unit.py and test_fields_numbers_unassigned_enum.py give 32 passed, 1305 subtests.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict carried to 777157480: GOOD TO GO. This is a pure rebase onto main a608b6e83, after #1499 merged.

  • The range-diff shows =, and the patch-id is unchanged at 5746b3a21f13.
  • On the rebased tree, tests/corekit plus the PR's test files gave 504 passed, 16 skipped and 4579 subtests passed, with pcapkit.__file__ asserted inside the tree.

@JarryShaw
JarryShaw force-pushed the fix/1488-numberfield-bit-length branch from 8da3cbd to 7771574 Compare October 9, 2026 21:05
#1488)

- NumberField.post_process: when the octets carry bits outside bit_length
  that pre_process would not write back (zeros, or a negative signed value's
  sign extension), return an int subclass recording them with the capturing
  field's length, bit_length, sign and byte order. pre_process writes them
  back only for a field matching all four; any other field, and any other
  value, is truncated to bit_length bits as before.
- EnumField.post_process: carry that record on the pseudo-member, or on an
  unregistered copy of a registry member, which stays isinstance and equal.
- Delete the Gap(1488) row; relax one assertIs pin to isinstance/equality
  for an octet whose spare bits are set.
@JarryShaw
JarryShaw force-pushed the fix/1488-numberfield-bit-length branch from 7771574 to db3b2ec Compare October 9, 2026 21:24
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict carried to db3b2ecad: GOOD TO GO. This is a pure rebase onto main 6af87f608, after #1509 merged. The range-diff is =, the patch-id is unchanged at 5746b3a21f13, and main only touched extraction.py. The PR's three test files pass on the rebased tree: 32 passed, 1305 subtests.

@JarryShaw
JarryShaw merged commit 5f4da23 into main Oct 9, 2026
35 checks passed
@JarryShaw
JarryShaw deleted the fix/1488-numberfield-bit-length branch October 9, 2026 21:50
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 9, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(corekit): a NumberField whose bit_length is narrower than its octets drops the high bits

1 participant