From 96036b627527a16347ff0480d529f4e480ae7729 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Sat, 25 Jul 2026 10:04:10 +0200 Subject: [PATCH] refactor(socketwatch): carry only the pid and port a socket event is for SocketEvent carried proto, remote_ip, remote_port and outbound "for the connection log later". Nothing ever read them, and the review found out why that never hurt: for all four, the NETWORK-layer packet the engine already holds is a strictly better source. The connection log takes the remote address, port, proto and direction straight off the packet, and the packet even distinguishes ICMP, which the SOCKET layer does not. The one thing a packet cannot tell us is the owning pid, and that is what the event still carries. Keeping them was not free. _ipv4() ran per event for nobody, and it decodes IPv4 ONLY (addr[0] as a 32-bit int) - so the first person to "just wire up the field we already have" would have shipped garbage for IPv6 into a user-visible column, in a tool whose traffic filters all cover v4 and v6. The dead field was a trap with a plausible-looking comment on it, which is the shape of drift this project keeps paying for. - SocketEvent is now "kind pid local_port"; _ipv4() and its test are gone - the octet finding from that test is preserved in CHANGELOG-INTERNAL.md: the spike proved WinDivert stores the IPv4 addr MSB-first, and the naive decode rendered 192.168.1.29 as 29.1.168.192. The code goes, the knowledge stays - event helpers updated in the three socketwatch test files - OPEN_PENDING is empty again, which is its healthy state (convention 44) - guard fix found BY this closing: test_no_stale_pending_markers now skips CHANGELOG*.md, because a changelog records what HAPPENED and stays true after the marker it announced is gone. Re-verified by mutation afterwards: a marker naming an unlisted stage in an ordinary .md still turns the test red, so the scan was narrowed rather than broken Co-Authored-By: Claude Opus 5 --- CHANGELOG-INTERNAL.md | 27 +++++++++++++++++++++++ beantester/socketwatch.py | 34 ++++++++--------------------- tests/test_repo_conventions.py | 16 +++++++++----- tests/test_socketwatch.py | 16 +++----------- tests/test_socketwatch_wiring.py | 2 +- tests/test_targeting_socketwatch.py | 2 +- 6 files changed, 52 insertions(+), 45 deletions(-) diff --git a/CHANGELOG-INTERNAL.md b/CHANGELOG-INTERNAL.md index c51104a..5eb8658 100644 --- a/CHANGELOG-INTERNAL.md +++ b/CHANGELOG-INTERNAL.md @@ -42,6 +42,33 @@ a `### BREAKING` section placed FIRST in that version, and each such line is pre - Help text and the flag tables in both READMEs now state that the flag is valid on its own. - Version bump deliberately NOT taken (convention 34): the owner closes it in `VERSION.txt`. +### Changed: SocketEvent carries only what the map is for (closes socket-event-fields) + +The SOCKET-layer event used to carry `proto`, `remote_ip`, `remote_port` and `outbound` "for the +connection log later". Nothing ever read them, and an engineering review found out why that never +became a problem worth solving: for all four, the NETWORK-layer packet the engine already holds is +a strictly better source. The connection log takes `remote_ip` / `remote_port` / `proto` / direction +straight off the packet, and the packet even distinguishes ICMP, which the SOCKET layer does not. +The one thing a packet cannot tell us is the owning pid - which is precisely what is left. + +- `socketwatch.py`: `SocketEvent` is now `kind pid local_port`. `_ipv4()` goes with it, so the real + Windows source no longer decodes an address per event for nobody. +- Dropping `_ipv4()` also removes a live TRAP. It decodes IPv4 only (`addr[0]` read as a 32-bit + int), so the first person to "just wire up the field we already have" would have shipped garbage + for IPv6 into a user-visible column, in a tool whose traffic filters all cover v4 AND v6. +- PRESERVED from the deleted `test_ipv4_decodes_high_byte_first`, because the code is gone but the + knowledge should not be: the 2026-07-22 spike established that WinDivert stores the IPv4 address + MSB-first in `addr[0]`, and that the naive low-byte-first decode rendered 192.168.1.29 as + 29.1.168.192. If remote addresses are ever wanted again, start there - and extend it to IPv6. +- Helpers updated in `test_socketwatch.py`, `test_socketwatch_wiring.py` and + `test_targeting_socketwatch.py`. `OPEN_PENDING` is now empty, which is its healthy state. +- Guard fix, found BY this closing: `test_no_stale_pending_markers` now skips `CHANGELOG*.md`. A + changelog records what HAPPENED and is dated by its nature, so the entry below that announced the + marker stays true after the marker is gone - flagging it was a false positive, and the first real + closing walked straight into it. Re-verified by mutation after the fix: a marker naming an + unlisted stage in an ordinary `.md` still turns the test red, so the scan was narrowed, not + broken. + ### Tests: a mechanical guard for prose with an expiry date (convention 44) The 2b/2c drift fixed below was not a set of FALSE claims - it was four claims that were true when diff --git a/beantester/socketwatch.py b/beantester/socketwatch.py index 91bebc2..ef43894 100644 --- a/beantester/socketwatch.py +++ b/beantester/socketwatch.py @@ -55,26 +55,13 @@ _ADD = frozenset({BIND, CONNECT, LISTEN, ACCEPT}) # One socket-layer event, normalised away from the ctypes struct so the map logic -# (and its tests) never touch pydivert. The map keys on local_port alone: proto, -# remote_ip, remote_port and outbound are carried but NOT consumed anywhere yet, -# and whether the connection log should use them or they should be cut is still -# open - PENDING(socket-event-fields). -SocketEvent = namedtuple( - "SocketEvent", "kind pid proto local_port remote_ip remote_port outbound") - - -def _ipv4(addr): - """WinDivert stores the IPv4 address in ``addr[0]``, MSB = first octet. - - Verified against a known local IP in the 2026-07-22 spike: the naive - low-byte-first decode produced ``192.168.1.29`` reversed as ``29.1.168.192``, - so the octets are read high-to-low here. - """ - try: - v = int(addr[0]) & 0xFFFFFFFF - except Exception: - return "" - return ".".join(str((v >> s) & 0xFF) for s in (24, 16, 8, 0)) +# (and its tests) never touch pydivert. An event carries only what this map is for: +# WHO owns a local port. It used to also carry proto / remote_ip / remote_port / +# outbound "for the connection log later", and nothing ever read them - because the +# NETWORK-layer packet the engine already holds is a strictly better source for all +# four (it even distinguishes ICMP, which the SOCKET layer does not). The one thing +# a packet cannot tell us is the owning pid, which is exactly what is left here. +SocketEvent = namedtuple("SocketEvent", "kind pid local_port") class SocketWatcher: @@ -247,11 +234,8 @@ def __iter__(self): sock = pkt.socket if sock is None: continue - yield SocketEvent( - kind=int(pkt.event), pid=int(sock.ProcessId), - proto=int(sock.Protocol), local_port=int(sock.LocalPort), - remote_ip=_ipv4(sock.RemoteAddr), remote_port=int(sock.RemotePort), - outbound=bool(pkt.is_outbound)) + yield SocketEvent(kind=int(pkt.event), pid=int(sock.ProcessId), + local_port=int(sock.LocalPort)) def close(self): with crashlog.quiet("socketwatch.source.close"): diff --git a/tests/test_repo_conventions.py b/tests/test_repo_conventions.py index 31bba01..bcdfd65 100644 --- a/tests/test_repo_conventions.py +++ b/tests/test_repo_conventions.py @@ -155,11 +155,10 @@ def test_no_em_or_en_dashes_in_repo_text(): # This set is NOT a roadmap. It is the set of stage ids that PROSE currently points # at, which is why an id nothing references is a failure too: it means the marker is # gone and the entry was left behind. -OPEN_PENDING = { - # SocketEvent carries remote_ip / remote_port / proto / outbound, but the map - # consumes only kind / pid / local_port. Use them or cut them - undecided. - "socket-event-fields", -} +# Empty is the HEALTHY state: it means no prose in the repo is currently waiting on a +# stage. Add an id the moment you write a sentence with an expiry date, and delete it +# when the stage lands - the second check below then points at whatever prose is left. +OPEN_PENDING = set() PENDING_EXTS = (".py", ".md") @@ -190,6 +189,13 @@ def test_no_stale_pending_markers(): continue if name == os.path.basename(__file__): continue # the registry does not scan itself + if name.startswith("CHANGELOG"): + # A changelog records what HAPPENED and is dated by its nature: an + # entry saying "added a marker named X" stays TRUE after X closes, so + # it is history, not drift. Found the hard way - the first real + # closing (socket-event-fields) tripped over the very entry that + # announced the marker. + continue path = os.path.join(dirpath, name) text = open(path, encoding="utf-8", errors="replace").read() for lineno, line in enumerate(text.splitlines(), 1): diff --git a/tests/test_socketwatch.py b/tests/test_socketwatch.py index cbee87b..7857d91 100644 --- a/tests/test_socketwatch.py +++ b/tests/test_socketwatch.py @@ -11,12 +11,12 @@ import time from beantester.socketwatch import (ACCEPT, BIND, CLOSE, CONNECT, LISTEN, - SocketEvent, SocketWatcher, _ipv4) + SocketEvent, SocketWatcher) from fakes import check -def ev(kind, pid, port, proto=6, remote_ip="1.2.3.4", remote_port=443, outbound=True): - return SocketEvent(kind, pid, proto, port, remote_ip, remote_port, outbound) +def ev(kind, pid, port): + return SocketEvent(kind, pid, port) class _FakeNames: @@ -229,13 +229,3 @@ def close(self): time.sleep(0.05) check("a mid-run failure IS recorded", len(recorded) == 1, f"({recorded})") w2.stop() - - -# -- the address decode locked by the spike ----------------------------------- # -def test_ipv4_decodes_high_byte_first(): - """The 2026-07-22 spike proved WinDivert stores the IPv4 addr MSB-first in - addr[0]; the naive low-byte-first decode printed 192.168.1.29 as - 29.1.168.192. This locks the correct order.""" - check("ipv4 reads MSB-first", _ipv4([0xC0A8011D, 0, 0, 0]) == "192.168.1.29", - f"({_ipv4([0xC0A8011D, 0, 0, 0])})") - check("a bad addr array is empty, not an exception", _ipv4(None) == "") diff --git a/tests/test_socketwatch_wiring.py b/tests/test_socketwatch_wiring.py index 66e37dd..5fbb633 100644 --- a/tests/test_socketwatch_wiring.py +++ b/tests/test_socketwatch_wiring.py @@ -27,7 +27,7 @@ def _wait(pred, timeout=5.0): def ev(kind, pid, port): - return SocketEvent(kind, pid, 6, port, "1.2.3.4", 443, True) + return SocketEvent(kind, pid, port) class _FakeSocketSource: diff --git a/tests/test_targeting_socketwatch.py b/tests/test_targeting_socketwatch.py index 22ef411..ae3ea0e 100644 --- a/tests/test_targeting_socketwatch.py +++ b/tests/test_targeting_socketwatch.py @@ -29,7 +29,7 @@ def _wait(pred, timeout=5.0): def ev(kind, pid, port): - return SocketEvent(kind, pid, 6, port, "1.2.3.4", 443, True) + return SocketEvent(kind, pid, port) class _Names: