fix(fields): raise ProtocolError for a NumberField negative resolved length (#828)

[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• make test passes, and a test case covers the change — did not run the full suite (it OOMs on this host); ran the scoped tests/corekit/ + HIP protocol unit tests instead (see below), and a new test case covers the change
• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A, changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• 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
• chore — anything else

---

Description of your pull request and other information

Closes #828

NumberField.__call__ cached bit_length = length * 8 and shifted 1 << bit_length before FieldBase.length's own negative-length guard (#805/#811/#827) or build_template ever saw the value. A length callback resolving below zero — e.g. NumberField(length=lambda pkt: pkt['len'] - 4, signed=False) at hip.py:734, against a truncated wire len — raised a bare, uncatchable ValueError: negative shift count.

Guards the resolved length in __call__, before the shift, raising ProtocolError worded as a sibling of FieldBase.length's message. No sibling field type (strings/misc/collections) does this eager shift, so the guard stays local to NumberField. A resolved length of exactly 0 is untouched and still succeeds.

Adds tests/corekit/test_fields_numbers_negative_length.py, verified failing on stock main with the reported ValueError and passing after the fix. Ran tests/corekit/ (226 tests) and the HIP protocol unit tests (44 tests) via plain unittest, all passing. coverage run -m pytest on numbers.py stays at 100% (140→142 stmts, 40→42 branches).

[JarryShaw]

Cross-review verdict: GOOD TO GO (haiku; author was sonnet). The guard is sound, and the review refutes the author's supporting argument — which I reproduced.

There is a second, unguarded shift in the same file. The author argued NumberField has one eager shift; it has two. numbers.py:88 in __init__ does self._bit_mask = (1 << bit_length) - 1:

NumberField(length=4, bit_length=-1) -> ValueError: negative shift count   is BaseError=False
control, bit_length=32               -> ok, template='>I'

That is exactly the #828 defect shape surviving this PR. Not a blocker: bit_length is never a callable, and all three in-tree sites pass positive literals (vlan.py:47 bit_length=3, :49 =1, :51 =12), so it is unreachable from wire data and reachable only by a programmer typo. Filing it separately.

Also latent, and I confirmed the surprising half — a field supplying bit_length and a negative-resolving callable length skips the guard entirely (no raise at __call__), degrading to template='>-1s' with the error deferred to FieldBase.length. Still a ProtocolError, so not a correctness break, but one wire condition yields two different messages depending on an unrelated constructor argument. No in-tree site combines the two.

The guard itself cannot be bypassed for the shift it protects. The reviewer enumerated the whole condition space, and I confirmed no NumberField subclass overrides __call__ — all nine in numbers.py plus OptionEnumField/PortEnumField in the schemas inherit it.

A reason for length= over template= that the author did not give, and it is the decisive one. A template is available at the guard — but for a callable-length field __init__ built it from the placeholder -1, so new_self.template is '>-1s' regardless of the actual resolved value. Reporting template= there would have been actively misleading; length= is the only accurate key. The shared prefix Field {name} resolved to a negative length; is the real contract, and it stays a sibling after #827 lands, which splits off only has a malformed template;.

Two corrections to the record: the stock crash is at numbers.py:146 (_bit_mask = (1 << _bit_length) - 1), not at _bit_length = _length * 8 which does no shifting; and four of the six new tests fail without the fix, not six — the zero-boundary and positive-control tests pass on stock by design.

On over-pinning, contra my own worry: the tests are if anything too loose. They pin only the substring 'resolved to a negative length' and the bare numerals, not the full message or the <number> field name — which matters because that name is class-derived (field.py:585), so pinning it would have been the #827 trap exactly. assertIn('-1', …) would also pass on -10; 'length=-1' would tighten it. One dead assertion at :84, assertNotIsInstance(ctx.exception, type(None)), is trivially true — cosmetic.

Coverage independently confirmed at 142 statements / 42 branches / 0 missed / 100%, measured with a distinct COVERAGE_FILE and filtered at report time.