fix(corekit): reject a signed= that contradicts a field's fixed sign (#545)

[JarryShaw]

Closes #545.

The defect

All eight *IntField subclasses documented a signed constructor argument that
__signed__ then discarded — in both directions. Re-measured on this branch's
base 691f12ab5, in a repo venv with pcapkit.__file__ asserted to be the tree
under test:

UInt8Field(signed=True)._signed  = False
Int8Field(signed=False)._signed  = True
UInt8Field.__signed__            = False

A caller writing UInt32Field(signed=True) reasonably believes they asked for a
signed field and gets unsigned parsing with no warning. The values only look wrong
once the high bit is set, which for a length or an identifier may never happen in
testing — so the documentation is what invites the mistake, and deleting the
docstring line alone (option 1 in the issue) would leave the trap in place with its
advertisement removed.

The real count, by introspection rather than grep — every NumberField
subclass whose __signed__ is not None, and every one discards in its own
direction:

class	__signed__	before this change
Int8Field, Int16Field, Int32Field, Int64Field	True	(signed=False)._signed → True
UInt8Field, UInt16Field, UInt32Field, UInt64Field	False	(signed=True)._signed → False

EnumField and NumberField itself leave __signed__ unset, so their signed
argument was always real and stays so. PortEnumField/OptionEnumField in the
schema modules subclass EnumField without fixing a sign, so their docstrings were
already accurate and are untouched.

The decision: option 2

Rejecting, not documenting away and not honouring.

The grep that decided it. signed= appears at 12 field-construction sites in
the tree and every one of them targets the base NumberField, where
__signed__ is unset and the argument already worked:
schema/internet/hopopt.py:692,722, schema/internet/ipv6_opts.py:697,727,
schema/internet/hip.py:434,454,456,519,1003,1016,1308,1310,
schema/application/httpv2.py:91, schema/transport/tcp.py:744,749. No site
passes signed to one of the eight fixed subclasses, by keyword or positionally
(checked separately — there are no positional constructions of these classes at
all). So option 2 is a bug fix rather than a breaking change, and option 1 is not
forced.

Option 3 — letting signed= override __signed__ — was rejected: the point of
Int8Field versus UInt8Field is that the sign is fixed by the type, and making
it overridable reintroduces the ambiguity the split exists to remove. It would also
need __template__ to become overridable in lockstep, since the two encode the
same fact.

The exception: FieldValueError

• BoolError is wrong — it means "must be a bool", and the argument here is a
  perfectly good bool whose value contradicts the class.
• FieldError (a TypeError) is what this package raises for a missing or
  wrong-kind constructor argument — SchemaField.__init__ raises it as
  FieldError('Schema field must have a schema.').
• FieldValueError (a ValueError) is what it raises for a constructor
  argument whose value is impossible: BitField.__init__ already rejects an
  over-wide namespace entry with it, in the same directory and with the same
  f'{type(self).__name__}: …' message shape. Right type, impossible value, is
  ValueError territory in Python generally.

A second instance of the same shape, fixed in the same pass

__signed__ was resolved into self._signed, but the struct template was built
from the raw signed argument. A subclass that fixes __signed__ and leaves
__template__ unset — which the base class explicitly allows — therefore declared
itself signed and then unpacked unsigned. Measured on the base:

class Custom(NumberField):
    __length__ = 4
    __signed__ = True

Custom()._signed   = True
Custom()._template = '>I'      # wanted '>i'

so b'\xff\xff\xff\xff' parsed as 4294967295 rather than -1. Nothing in
pcapkit fixes __signed__ without __template__ today, so this was latent; the
line now reads self._signed, matching __call__ and pre_process, which already
did.

Also checked and not the same shape: __length__, where an explicit length=
wins over the class attribute rather than being discarded, so the argument is
documented truthfully. grepping the whole package for the
x if self.__attr__ is None else self.__attr__ pattern finds __signed__ and
nothing else.

The fix

signed defaults to None rather than False. That sentinel is what lets an
explicit contradiction be told apart from the default, and both directions need it —
Int8Field(signed=False) is otherwise indistinguishable from Int8Field(). A
contradiction raises FieldValueError naming the class and the sign it fixes; an
agreeing or omitted value behaves exactly as before. Judged on truth value, so
signed=1 contradicts an unsigned field just as signed=True does.

Evidence

tests/corekit/test_fields_numbers.py is new — this file had no test module at all,
which is why the defect survived. 12 tests.

Every run below used PYTHONSAFEPATH=1 with PYTHONPATH set to the worktree and a
pytest plugin that imports pcapkit in pytest_report_header and raises unless
pcapkit.__file__ is inside that tree, so no measurement here could have come from
the editable install. Exit codes read from a file, never from a pipeline.

tree	result	exit
this branch	12 passed	0
signed fix fully reverted	5 failed, 7 passed	1
reverted, -k unsigned_field_rejects	1 failed (UInt8Field(signed=True) was accepted)	1
reverted, -k signed_field_rejects	2 failed (Int8Field(signed=False) was accepted)	1
half-fix: rejects only the unsigned direction	3 failed, 9 passed	1
full fix, only the template line reverted	1 failed, 11 passed	1

The half-fix row is the point of testing both directions: it passes
test_an_unsigned_field_rejects_a_contradicting_signed_true and fails
test_a_signed_field_rejects_a_contradicting_signed_false. A test covering one
direction would have signed off on it.

Because this adds a rejection that a caller outside the tree could hit, the full
unit tier was run, not just the new module:

1048 passed, 8 skipped, 229 warnings in 894.21s        exit 0

isort --check-only is clean on both files. mypy and pylint each report exactly
one finding in numbers.py and both are pre-existing on untouched lines — the
enum.IntEnum('<unknown>', …) name mismatch in EnumField.post_process, and
W1309 on raise IntError(f'Field has no length.'). Neither gates CI, which runs
pytest only.

CI has not run on this branch. The runner has a deep backlog (~27 queued, 1 in
progress) and no PR branch has had a check start in over an hour, so the numbers
above are local.

Left for the owner

• FieldError's own docstring, "The argument(s) must be *field* type.", is
  narrower than its established use (SchemaField raises it for a missing
  argument). pcapkit/utilities/exceptions.py is not touched here.
• Int32Field(length=2) builds a field whose _length is 2 and whose _template
  is >i from __template__, so unpack raises struct.error — while __call__
  recomputes the template from the length and ignores __template__ entirely. The
  inverse trap to this one, and a separate issue.
• raise IntError(f'Field has no length.') at numbers.py:81 is an f-string with
  no interpolation, and IntError ("must be integral") is an odd fit for "no
  length was given". Left alone as unrelated to *IntField subclasses document a signed argument that __signed__ silently discards #545.

[JarryShaw]

✅ GOOD TO MERGE — independently reproduced all four fails-without rows exactly (fully reverted: 5 failed/7 passed; template-line-only reverted: 1 failed/11 passed; the half-fix that rejects only the unsigned direction: 3 failed/9 passed, exit 1 in every case), confirmed test_the_census_is_complete genuinely uses inspect.isclass/issubclass introspection over vars(numbers) rather than a hardcoded list, and confirmed the FieldError/FieldValueError distinction holds consistently across every raise site of both in the tree.

[JarryShaw]

Detailed review (independent verification, falsify-not-bless)

Head sha reviewed: b95b9191549be3cf9e8da4c2a621b3a9ab59c332. Per the coordinator's brief, the mechanical core (the 12-site grep showing no in-tree caller passes signed= to a fixed subclass, the exactly-8-fixed-sign-subclass count, and the second latent template-vs-__signed__ defect) was already independently verified and is not re-derived here; my effort went to the exception choice, completeness of the fix, the census test, and re-deriving the reported numbers.

The FieldError vs FieldValueError distinction -- checked against every use site in the tree, not just the two cited

grep -rn "raise FieldError(" pcapkit/ finds exactly 3 sites: SchemaField.__init__ ('Schema field must have a schema.', the PR's own citation -- a missing constructor argument) and two in pcapkit/protocols/schema/transport/tcp.py (mptcp_data_selector/mptcp_add_address_selector, both raising when a selector function cannot determine which field/schema type applies to the packet's own flags). The TCP pair is a slightly different shape than "missing argument" -- it is "no field type matches this input" -- but it is still a type/kind-selection failure rather than a value-of-a-correctly-typed-field failure, so it does not contradict the PR's TypeError-family framing.

grep -rn "raise FieldValueError(" pcapkit/ finds 45 sites (strings.py, collections.py, ipaddress.py x8, ipv4.py, and others): IP version mismatches, invalid lengths, invalid enum values, over-wide BitField namespace entries, missing schema/registry state -- every one of them is "the argument is the right kind, but its value is impossible for this field," matching the ValueError-family framing exactly. I found no site where the two are used interchangeably or contradictorily. The distinction the PR argues for holds up under a full-tree check, not just the two examples it cites.

BoolError's docstring ("The argument(s) must be bool type") independently confirms it would be backwards here, as the PR says -- rejecting a bool because its value is wrong is the opposite of "must be a bool."

Completeness of the fix -- both directions independently constructed, and the half-fix falsification reproduced exactly

Ran the clean test module first: tests/corekit/test_fields_numbers.py -- exit code 0, 12 passed, matching the PR's claim.

Then, rather than accepting the "both directions matter" claim, I wrote the half-fix myself: changed the guard to raise only when self.__signed__ is False and signed is True (rejecting the unsigned-fixed direction, silently falling through to self._signed = self.__signed__ for the signed-fixed direction). Ran the suite: exit code 1, 3 failed, 9 passed -- an exact match to the PR's claimed row. Confirmed by name that test_an_unsigned_field_rejects_a_contradicting_signed_true passes against this half-fix while test_a_signed_field_rejects_a_contradicting_signed_false fails -- exactly the asymmetry the PR describes, and decisive evidence that a test covering only one direction would have signed off on a half-fix. Reverted; diff against head empty afterward.

Also independently reproduced the "template line reverted alone" row: changed struct_fmt = self.build_template(self._length, self._signed) back to using the raw signed argument. Exit code 1, 1 failed, 11 passed -- test_a_class_fixing_a_sign_without_a_template_parses_with_that_sign fails with '>I' != '>i', exactly reproducing the second latent defect the coordinator already confirmed on main. Reverted cleanly.

And the full revert of both changes (git checkout main -- pcapkit/corekit/fields/numbers.py): exit code 1, 5 failed, 7 passed -- matches the PR's claimed row exactly, with the five failures spanning both the rejection tests and the template-sign test.

The census test -- confirmed to use introspection, not a hardcoded name list

Read test_the_census_is_complete directly: it builds found from {name: obj.__signed__ for name, obj in vars(numbers).items() if inspect.isclass(obj) and issubclass(obj, numbers.NumberField) and obj is not numbers.NumberField and obj.__signed__ is not None} and asserts it equals the module-level FIXED_SIGN dict. This is genuine introspection over the live module namespace: a ninth fixed-sign subclass added to numbers.py without a corresponding FIXED_SIGN entry would make found gain a key FIXED_SIGN lacks, failing the assertEqual rather than silently passing. The test does not merely hardcode eight class names on both sides of the comparison.

Reported numbers -- pylint and mypy independently re-run

• pylint (project's own Makefile:135 invocation) on numbers.py: exactly 1 real code message, W1309 (f-string-without-interpolation) at the raise IntError(f'Field has no length.') line -- confirmed identical (same text, shifted line number 65->81 by the diff) on main, so pre-existing and untouched.
• mypy (project's Makefile:138 invocation) on the whole package, filtered to numbers.py: exactly 1 finding, the enum.IntEnum(...) name-mismatch [misc] in EnumField.post_process -- confirmed identical (line 382->461) on main, so pre-existing and untouched.

Full unit tier

Ran pytest tests --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py' in the background. One run was contaminated by my own file-swapping mid-execution during the pylint/mypy before/after checks (an early F appeared at ~2% before I noticed and killed it) -- restarted cleanly on a stable checkout and it is still running as I write this; will post the exact final numbers as a follow-up once it completes, same as I did on a prior PR in this review round when my own number differed in scope from the PR's.

CI status

Not run -- backed up throughout this review session; verdict on local evidence only, per standing instruction.

What remains unverified

• The full unit-tier run had not completed at the time of this write-up; a follow-up comment with the exact number is coming.
• The 12-site grep and the exactly-8-subclass count were taken from the coordinator's own independent verification rather than re-derived a third time, per the explicit instruction not to re-spend budget there.
• isort --check-only on the two changed files was not independently re-run.

[JarryShaw]

Follow-up on the full-suite number promised above (the first run was contaminated by
my own file-swapping mid-execution and was discarded; this is the clean rerun):

pytest tests --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'
on b95b91915: EXIT=0, 1051 passed, 5 skipped, 2524 subtests passed in 904s (~15 min).

Close to but not identical to the PR's claimed "1048 passed, 8 skipped": the totals match
exactly (1051+5 = 1048+8 = 1056), so this reads as an environment-dependent shift in which
few tests get skipped (likely a runtime-dependency or platform-conditional skip that
resolves differently between environments) rather than any test actually failing or
disappearing. Exit code 0, zero failures, consistent with everything else verified above.