Skip to content

fix(tcp): NS flag is written by make() but never parsed - #1114

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1098-tcp-ns-flag
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1098-tcp-ns-flag

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • Searched for similar pull requests
  • Followed the coding style: isort and mypy are clean on the changed files; pylint only repeats the existing unused-argument pattern on the Flags.__init__ stub
  • make test passes, and a test case covers the change. Not run in full: see the module list below
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1098. NS was commented out of Flags, the reader and _make_data in 2e39aeb99 ("minor revision for TCP on typings"). The commit gives no reason, and it kept NS in make() and in the OffsetFlag schema, so this PR makes NS round-trip rather than removing it. The owner ruled on #1098 to expose NS. The tcp.rst footnote saying NS is "not surfaced in Flags … left unexposed" is removed together with its [*]_ marker. The tcp.flags.ns row stays, and the one remaining [*] footnote still pairs with its marker.

Probe (TCP(srcport=1, dstport=2, ns=True), parse it, then rebuild with from_data): before, byte 12 was 0x51, 'ns' in flags was False, and the rebuild gave 0x50. After: 0x51, flags.ns is True, and the rebuild gives 0x51.

Tests: new test_tcp_ns_flag_roundtrip_unit.py: 4 passed with the fix. With the fix reverted, 1 failed plus 4 subtests failed (read, from_data, field order); make_sets_ns passes either way, because it guards the write side. In test_tcp_udp_unit.py, the hand-built Flags stand-in pinned the old 8-field shape, so ns=False was added to it (18 passed). Also green: test_option_roundtrip_unit (no EXPECTED_FAILURES change), test_option_generator_tcp_base_unit, and the six test_tcp_mptcp_*_unit modules. tests/project: 379 passed, 1 skipped.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 80b62ede5: NEEDS CHANGES (ran on Sonnet; author Opus)

The code is correct:

  • NS is bit 7 of byte 12. Probed with 0x51 and 0x5e, and it does not leak into CWR.
  • from_data is now byte-identical.
  • With the change reverted, the new module fails 5 tests.

But docs/source/pcapkit/protocols/transport/tcp.rst:538-541 still says NS is deliberately not surfaced in Flags, which is now false. Whether to expose NS at all is on hold for the owner (#1098), so this waits on that decision.

@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 6, 2026
make() packed NS into bit 103 and the OffsetFlag schema declared it, but
the reader and _make_data had ns commented out (2e39aeb, a typing-only
pass with no stated reason), so a parsed segment had no flags.ns and a
from_data rebuild cleared the bit silently.

- data/transport/tcp.py: restore ns on the Flags data model
- transport/tcp.py: read ns from schema.offset, emit it in _make_data,
  and list tcp.flags.ns in the module's header table
- tcp.rst: drop the footnote saying NS is left unexposed
- test_tcp_udp_unit.py: add ns to the hand-built Flags stand-in
- new test_tcp_ns_flag_roundtrip_unit.py

Closes #1098
@JarryShaw
JarryShaw force-pushed the fix/1098-tcp-ns-flag branch from 80b62ed to 45e84f0 Compare October 6, 2026 19:10
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one 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 review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 45e84f07e: GOOD TO GO (ran on Sonnet; author Opus, round 2)

Round 1's only finding is fixed. The footnote in tcp.rst that said NS is deliberately hidden is gone, and the tcp.flags.ns row stays. That follows the maintainer's ruling on #1098 to expose NS.

The one remaining [*]_ marker pairs with the one remaining footnote. The delta is that single .rst file; the code and tests match round 1.

Round-1 evidence still holds:

  • NS is bit 7 of byte 12, and setting it does not leak into CWR.
  • from_data round-trips byte-identically.
  • With the fix reverted, the new module fails 5 tests.

@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 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.74% (unit tier, Python 3.14, 45e84f07e, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1874 91 578 22 94.33%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2422 143 842 34 92.62%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15653 187 3942 162 98.19%
pcapkit/toolkit 487 71 144 3 84.15%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

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

@JarryShaw
JarryShaw merged commit eeed87f into main Oct 6, 2026
42 checks passed
@JarryShaw
JarryShaw deleted the fix/1098-tcp-ns-flag branch October 6, 2026 20:35
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 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(tcp): NS flag is written by make() but never parsed

1 participant