fix(tcp): drop the doubled separator from four MPTCP error messages (#649) - #671
Conversation
…s in #670 and #671 Two bullets, not one, because the two defects are unrelated: one changes what the dumpers emit, the other only the text of four exception messages. They share a file only by accident of being found in the same pass. The #648 bullet leads with the output change and says so in bold, because that is what a reader upgrading needs to see: a flag value with no declared bits dumped as `Type::None [0]` in all six textual format names, out of both `Extractor` and `TraceFlow`. It then justifies the *replacement* rather than just stating it, since "render it as its decimal value" looks arbitrary until you know the enumeration libraries already spell an undeclared residue that way -- and that a decimal cannot collide with a member name where `None` can, `NONE` being a real declared name elsewhere. Three things a reader would otherwise get wrong are recorded: three sites carried the interpolation and not one, the guard is on `name is None` rather than on zero because the defect never was about zero, and it is not an `aenum` quirk since stdlib `enum.IntFlag` behaves identically. It also corrects the issue on a point of fact. #648 said `Flags` was the only registry nameless at zero; a sweep of all seven finds five, the four Mobility Header flag registries included. And it states that the committed example dumps do not move, which was measured by regenerating all three with and without the change rather than assumed -- a reader of a bullet this emphatic will otherwise wonder whether `examples/captures/` drifted. The #649 bullet says "cosmetic" in its second sentence so nobody reads it as a behavioural change, then gives the one reason it was worth doing at all: it is the text a user sees when an option is rejected. It names all four sites, and the 28-against-4 count in the same file, because that count is what makes the correct form a fact about the module rather than a preference. Neither bullet claims a guard it does not have: #648's third site, the `addon` branch, is not reachable from any registry in the library today, and the bullet does not imply otherwise. `CHANGELOG.md` regenerated with `util/changelog_md.py`, not edited by hand. `--check` exits 0 and `tests/project/` is green at 96 passed, 469 subtests. Each `` literal is kept on one line, since the generator rejects one spanning a line break with `ResidualMarkupError`. Committed from a detached HEAD on d14577d and pushed to the branch ref, because `docs/changelog-1.5.0` is checked out in another agent's worktree and could not be taken here. Note d14577d, not the 69a6e13 I was given: the branch had already moved on with #665's, #651's, #661's and #652/#650's entries. Refs #648 Refs #649
…649) Four `ProtocolError` messages in the MPTCP option handlers were spelled `f'{self.alias}: : [OptNo …]'`, so they rendered with an empty field between the protocol alias and the option number: TCP: : [OptNo 30] 1: invalid flags combination Cosmetic -- the exception type, the option number and the subtype were always right, and nothing downstream parses these strings -- but it is the string a user sees when an MP_JOIN or DSS option is rejected, and an empty field reads as a value that failed to interpolate. * Four sites, not the one it was reported against. `_read_mptcp_join` and `_make_mptcp_join` ("invalid flags combination"), and both guards of `_make_mptcp_dss` ("missing required fields"). The other three were found by grepping for the pattern; `grep -rn "alias}: : " pcapkit/` returns nothing outside this file. * The correct form is the local majority, not a preference: the same file spells the same prefix without the doubling 28 other times, all `f'{self.alias}: [OptNo {schema.kind}] …'`. The count is 32 after this change and the doubled count is 0. * New `test_tcp_mptcp_error_message_unit.py` asserts each message in **full** rather than by substring, which is the point: the two places that already exercise these messages both match a substring straddling the defect and so pass either side of it -- `test_tcp_mptcp_join_flag_ordering_unit.py:404` (`assertIn`, against `_make_mptcp_join`) and `test_tcp_udp_unit.py:1629` (`assertRaisesRegex`, against `_read_mptcp_join`). Two, not one. Neither module is touched. * The two `_make_mptcp_dss` guards carry the identical message, so they are reached separately -- `dsn` without its data-level fields, and a data-level field without a `dsn` -- since asserting the string once would not show both were fixed. Coverage confirms all four `raise` lines execute. * The test also sweeps the source for the pattern and pins the 32, so a fifth site cannot be added unnoticed and "fixing" the four by deleting them would not pass. That exactness makes it a tripwire on the whole module, so the assertion carries a note telling a future editor to update the number rather than loosen it. All four lines arrived together in `3bba8a1748` (2023-04-10) and were untouched until now. `tests/protocols/transport/` unit tier passes 140 tests and 105 subtests, up from 136 and 103. The new module is 4 tests and 2 subtests, and fails on `main` with `'TCP: : [OptNo 30] 1: invalid flags combination' != 'TCP: [OptNo 30] 1: invalid flags combination'` among 5 failures. `pcapkit/protocols/transport/tcp.py` holds 100% statement and branch coverage either side, at an unchanged 594 statements and 198 branches -- the change is to string literals, so it adds no statement to cover. The seven `test_tcp_runtime.py`/`test_udp_runtime.py` failures in the full directory are the fixture-dependent tier raising `FileNotFoundError` in a tree where `make samples` has not been run, and are unaffected by this commit. Fixes #649
afa0216 to
0e616ac
Compare
Cross-review verdict: GOOD TO GOIndependent cross-review, run on a different model (Sonnet) from the one that authored the change, in its own worktree, briefed to falsify rather than bless and to treat the issue's own measurements as claims rather than ground truth. It was read-only with respect to this PR: no amend, no push, no commit to the branch. Tree it measured: Per-claim verdicts
Structural checks also passed: one commit, author What it disputed, and what I changedBoth findings acted on rather than argued away. The commit was amended and the branch force-pushed; the fix itself is unchanged.
It also observed that this branch looked 5 commits behind Nothing in A–J was left unverified. |
1 participant
Fixes #649.
Cosmetic. No behavioural change: the exception type, the option number and the subtype were always correct, and nothing downstream parses these strings. What changes is the rendered text a user sees when an MP_JOIN or DSS option is rejected.
The defect
Four
ProtocolErrormessages in the MPTCP option handlers were spelledf'{self.alias}: : [OptNo …]', so they rendered with an empty field between the protocol alias and the option number:An empty field reads as a value that failed to interpolate, which sends a reader looking for a missing variable that was never there.
All four sites
All in
pcapkit/protocols/transport/tcp.py. Line numbers had shifted from the issue's by 13, so they are given as found::1577_read_mptcp_joinTCP: : [OptNo 30] 1: invalid flags combinationTCP: [OptNo 30] 1: invalid flags combination:2831_make_mptcp_joinTCP: : [OptNo 30] 1: invalid flags combinationTCP: [OptNo 30] 1: invalid flags combination:2997_make_mptcp_dssTCP: : [OptNo 30] 2: missing required fieldsTCP: [OptNo 30] 2: missing required fields:2999_make_mptcp_dssTCP: : [OptNo 30] 2: missing required fieldsTCP: [OptNo 30] 2: missing required fields30isOption.Multipath_TCP,1isMPTCPOption.MP_JOINand2isMPTCPOption.DSS; all three format as their integer value. The before/after strings above are rendered by execution, not read off the f-strings.Why this form, and not a preference
The correct form is the local majority in the very same file:
The four were outliers against a 28-site house form in their own module, and the fix moves them into it.
grep -rn "alias}: : " pcapkit/returns nothing outside this file, so the defect is confined to these four lines and is now gone from the package entirely. Two sibling test modules already quote the correct single-separator form in their docstrings (test_tcp_mptcp_subtype_unit.py:279,test_tcp_mptcp_capable_length_unit.py:195), which is further evidence of the intended shape.The test, and why a substring assertion would not have done
New module
tests/protocols/transport/test_tcp_mptcp_error_message_unit.py, 4 tests / 2 subtests. Every message is asserted in full, and that is the point of the change. Two existing assertions already exercise these messages and neither could have caught this, because both match a substring that straddles the defect:Both pass identically before and after this fix. Both modules are left untouched — a new module rather than an edit to either, given both have been repaired recently (#627, #628, #634).
(Corrected after cross-review: an earlier draft of this description named only the first of the two.)
Four things the module does deliberately:
_make_mptcp_dssguards separately —dsnwithout its data-level fields, and a data-level field without adsn. They carry the identical message, so asserting the string once would not show that both lines were fixed._make_*sites,TCP(syn=…, ack=…, options=[(Option.Multipath_TCP, …)], …), rather than poking a bareobject.__new__(TCP)._read_mptcp_joinrather than hand-writing_flags, for the reason test_tcp_udp_unit.py assigns a plain set to TCP._flags, which is how #587 hid behind 100% coverage #603/test(tcp): reach the MP_JOIN dispatchers through TCP(), not a hand-written _flags (#603) #612 give. That branch is not reachable from a caller —mptcp_data_selectorrejects a flagless MP_JOIN with aFieldErrorfirst — so it is latent, but latent only by virtue of a guard in a different file, and the string is still what a caller reaching it sees. Noted in the test rather than left to be rediscovered.== 0on the doubled form above it is the one that must hold forever. Raised by the cross-review as a footgun and kept deliberately, with the warning added.Failing, then passing
Same harness as its sibling PR:
__editable__*stripped fromsys.meta_path, worktree atsys.path[0],pcapkit.__file__asserted and printed before any other import, and pytest's exit code read from a file rather than a pipeline.On
main(fix reverted, test present):All four tests fail, covering all four sites.
With the fix:
Scoped directory run,
tests/protocols/transport/unit tier:The full directory additionally shows 7 failures, all in
test_tcp_runtime.py/test_udp_runtime.py, allFileNotFoundErrorfor generated captures in a tree wheremake sampleshas not been run. That is the fixture-dependent tier documented intests/_tiers.py, and it is unaffected by this change.The suite was never run whole; every run was scoped.
Coverage
pcapkit/protocols/transport/tcp.py, statement and branch:The four changed lines already executed, so per the usual rule the subtest count is the number that moves: 103 → 105, with tests 136 → 140. The statement and branch counts are necessarily unchanged because the change is to string literals inside existing
raisestatements, and coverage holds at 100%.Provenance
All four lines arrived together in
3bba8a1748(2023-04-10, "revised TCP schemas with OptionField/etc. & redesigned read funcs") and were untouched by any later change.Label
fixalone. Notbreaking: no public API, wire output or dump output changes — only the text of an exception message, and nothing in the package or the tests parses it.Cross-review
Dispatched on a different model, briefed to falsify rather than bless; verdict posted as a comment below.