fix(fields): distinguish a malformed template from a negative length in FieldBase.length

[JarryShaw]

• 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
• Added a changelog entry — 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

---

Description

struct.calcsize raises the identical bare struct.error for a malformed
template as for a negative resolved count (calcsize('-1s') and
calcsize('Xs') both raise "bad char in struct format"). #811's guard
caught that blanket and always reported "resolved to a negative length",
misdiagnosing a typo'd template as the wrong category of error.

FieldBase.length now tells the two apart once struct.calcsize has
failed, using a pattern match against the template itself (every template
this package builds is f'{length}s'; a negative count is the only way
one starts with -). Both branches still chain from the real
struct.error, so neither leaks a bare one, and the negative-length
message and __cause__ are unchanged for the existing case.

Also measured (not fixed here): whether the negative length should be
prevented rather than just correctly reported. Both a global clamp in
schema.py's decrementing and a narrow one on the two affected fields
still leave the GOAWAY repro raising a different ProtocolError
("invalid format", from httpv2.py's own length guard) — so it needs its
own change, not a hasty one here.

Closes #825

[JarryShaw]

Cross-review verdict: NEEDS CHANGES (haiku; author was sonnet). One real defect, which I reproduced.

^-\d+ misses every byte-order-prefixed negative length — which is the case this PR exists to fix. NumberField builds its template as f'{endian}{struct_fmt}' (numbers.py:121, :151) over the f'{length}s' fall-through at :195, so a negative resolved length yields '>-1s', not '-1s':

NumberField(length=-1)                   template='>-1s' -> "Field <number> has a malformed template; template='>-1s'"
UInt32Field(callable -1, bit_length=32)   template='>-1s' -> "Field <uint32> has a malformed template; template='>-1s'"

So the #825 misdiagnosis is reintroduced in the opposite direction. The fix is one character class — re.compile(r'^[@=<>!]?-\d+') — and I verified it flips exactly the six endian-prefixed negatives (>, <, !, =, @) while moving no control: 'Xs', 'Qz', '-', 's-1', ' -1s', '+1s', '>Xs', '--1s' all stay malformed.

The comment's reasoning needs correcting too, not just the regex. It argues a byte-order prefix "is always one of @=<>!, never -" — true and irrelevant: the prefix precedes the minus, which is what defeats the ^ anchor. It also says every template built from a resolved length is f'{length}s', citing strings, misc and collections — but numbers.py:195 does that too and then prefixes it, which is precisely the omission that produced the defect.

Confirmed and not in dispute: main does not go red (the pinned assertion at test_http_unit.py:663 passes, 1 passed); no bare struct.error escapes either branch, with __cause__ verified on the malformed case too; the two new tests fail on stock 21e9588af with the exact claimed error; and the new tests use substring assertions rather than pinning the full message, so they do not repeat the brittleness that caused this class of problem. Scope is exactly two files. Coverage is UNVERIFIED — its measurement never completed.

Deferring defect (2) is justified, and the evidence is stronger than the author's. Applying the narrow max(pkt['__length__'], 0) bound makes the GOAWAY raise HTTP/2: [Type 7] invalid format with __cause__ of None — so bounding breaks main's pinned assertion three ways, including the assertIsInstance(ctx.exception.__cause__, struct.error) at :664. #825 keeps its follow-up.

Two corrections to my own brief, for the record: the pinned string is at :663 (the assertEqual opens at :662), and I wrote httpv2.HTTP where the test uses the http.HTTP dispatcher — a probe built on my wording returned HTTP/2: [Type 32] invalid format on both trees and failed its own control. Its reviewer caught that and rebuilt it.

Filing the escaping-ValueError finding as its own issue.

[JarryShaw]

What's exactly needing my decision?

[JarryShaw]

Nothing on this PR needs your decision. The needs: decision queue is empty repo-wide — your 02:13Z casing ruling was the last one, and it is implemented and verified on #815.

review: needs-changes here means an agent-fixable defect, not a question for you: the regex misses byte-order-prefixed negatives ('>-1s'), the one-character fix is verified, and a worker is applying it. I will not ask you about anything that has a determinate right answer.

What is actually waiting on you is merging. Three PRs are green and cleared:

PR	head	CI	note
#817	ed1240dcf	27✅ 0❌ 0inc	independent of the others
#821	555c4fcd2	27✅ 0❌ 0inc	shares http.py with #826
#657	1897ef5d7	27✅ 0❌ 0inc	green, but merges last by policy

One genuine design question I deferred rather than decided, if you want to settle it now — otherwise it stays in #825 for its own PR. #825 has two halves. The first (a malformed template misreported as a negative length) is fixed in #827. The second is that a negative pkt['__length__'] is only ever reported, never prevented: SchemaWarning: packet length < 0: -1 fires and the schema still builds a '-1s' template.

Bounding it at the schema layer is the obvious fix, and both ways of doing it were measured: the GOAWAY then raises HTTP/2: [Type 7] invalid format with __cause__ of None, so it changes what malformed input parses to and breaks the assertion main now pins. That is a behaviour call on a library that deliberately parses a lot of malformed input, which is why I did not make it. Bound it, or keep reporting-only?

[JarryShaw]

Fixed at 298daa247, verified by me with the probe that found the defect.

field.py:224  _RE_NEGATIVE_LENGTH_TEMPLATE = re.compile(r'^[@=<>!]?-\d+')

NumberField(length=-1)                    template='>-1s' -> "resolved to a negative length"   (was "malformed template")
UInt32Field(callable -1, bit_length=32)   template='>-1s' -> "resolved to a negative length"
controls, all still malformed: 'Xs' 'Qz' '-' 's-1' ' -1s' '+1s' '>Xs' '--1s'
main's pinned GOAWAY message -> identical? True, __cause__ = struct.error

So the endian-prefixed negatives are classified correctly, no control moved, and main's assertion is untouched.

The comment was corrected on both counts too: it now says the byte-order prefix precedes the minus and so defeats the ^ anchor — rather than the previous irrelevant framing that a prefix "is never -" — and it names numbers.py's build_template fall-through as the third site that builds f'{length}s' from a resolved length before wrapping it as f'{endian}{struct_fmt}'. That omission was what produced the defect, so naming it is the part that stops the next reader inheriting the same false premise.

The new test uses the real NumberField(length=-1) construction path rather than hand-setting _template, so it would also catch a future change to how templates are assembled. Shown failing on the old regex with AssertionError: 'negative length' not found in "Field number has a malformed template; template='>-1s'". tests/corekit 223 tests OK; mypy and isort clean.

review: pending at the new head; a short delta re-review is running. After that, CI is the only gate.

[JarryShaw]

Coverage is now confirmed, closing the one UNVERIFIED item from the review. Measured per tree with a separate COVERAGE_FILE so the two runs could not race:

tree	Stmts	Miss	Branch	Cover
stock 21e9588af	128	12	26	86%
PR 2bd98bf26	132	12	28	87%

86% → 87% with the missed count flat at 12, corroborated by test counts moving 220 → 222 — the two new tests and nothing else.

The reviewer's addendum restates NEEDS CHANGES, but that verdict is against 2bd98bf26 and predates the fix. It was still on the old head when it wrote it. The blocker it names is exactly the regex defect, which is fixed at 298daa247 and verified above: '>-1s' now reports "resolved to a negative length", all eight malformed controls unmoved, main's pinned message byte-identical. A delta re-review of the new head is running.

Three measurement traps it recorded, worth having on the record because each produced a confident wrong number before it caught them:

• pyproject.toml sets [tool.coverage.run] source = ["pcapkit"], which overrides a command-line --include — its first run measured the whole tree and printed TOTAL … 59%, which reads like a per-file figure and is not. Filtering has to happen at coverage report time.
• Two coverage runs in the same worktree both write the default .coverage concurrently, so either can report the other's data. It discarded both racing runs rather than reading a number off them.
• --source=<a .py path> fails silently with No data was collected / Module was never imported; source wants a package or directory.

[JarryShaw]

Delta re-review verdict: GOOD TO GO (haiku, same reviewer, on 298daa247 → now d3ae174fb after a content-identical rebase onto c287152d7).

I owe this comment — the verdict came in and I reported it in the session but never posted it here, so this PR has been reading review: pending with a clean verdict in hand. Recording it properly.

Its 28-template matrix had 0 mismatches, and its attacks on the widened pattern were all correctly refused: '>>-1s', '> -1s', '>-', '>-x', '<>-1s', 's>-1s', '2>-1s', '\t-1s'. Both real NumberField paths report the right category at template='>-1s', all eight original controls stay malformed, and main's pinned message is byte-identical with __cause__ a struct.error.

Two nuances it recorded, both of which I verified: struct ignores whitespace between formats (calcsize('> 4s') = 4, calcsize(' 4s') = 4, but calcsize('4 s') raises), so ' -1s' is semantically a negative count the regex calls malformed — unproducible here and identical under the old regex, so not a regression. And numbers.py only ever emits '>' or '<' (lines 112, 148, 246), never '@=!', so accepting all five is conservative rather than necessary.

Coverage confirmed 86% → 87%, misses flat at 12, corroborated by 220 → 222 tests.