From f6be268c9a052ee696686b42ff9c90e51d29e105 Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 16:22:50 +0200 Subject: [PATCH 1/2] test(packets): cover the TRANSPORT_DIRECT 0x00 marker and the #322 comment fixes Follow-ups to #313 (#282), tests first. - test-packets.js: a buildFieldTable case for a TRANSPORT_DIRECT frame (route 3, path byte at offset 5) whose 0x00 is the zero-hop marker. Kills the `(route === 2 || route === 3)` -> `(route === 2)` mutant that survived #313. - test-frontend-helpers.js: senderPathHashSize covers the route-3 0x00 marker too, and both buildFieldTable sandboxes expose the shared width helper. - test-issue-322-comment-guards.js (+ test-all.sh registration): source-text guards for points 3 (pktEsc leak header) and 4 (normalizeObservedPathHashSizes callers); red against the wrong wording, green once corrected in the next commit. Co-Authored-By: Claude Opus 4.8 --- test-all.sh | 1 + test-frontend-helpers.js | 11 ++++++ test-issue-322-comment-guards.js | 58 ++++++++++++++++++++++++++++++++ test-packets.js | 17 ++++++++++ 4 files changed, 87 insertions(+) create mode 100644 test-issue-322-comment-guards.js diff --git a/test-all.sh b/test-all.sh index cf8d1eeae..ddc846c6c 100755 --- a/test-all.sh +++ b/test-all.sh @@ -64,6 +64,7 @@ run test-issue-258-column-widths.js run test-issue-259-nodes-esc-listener.js run test-issue-282-pktesc-listener.js run test-issue-282-comment-guards.js +run test-issue-322-comment-guards.js run test-perf-go-runtime.js run test-channel-psk-ux.js run test-channel-sidebar-layout.js diff --git a/test-frontend-helpers.js b/test-frontend-helpers.js index 5d2c2c368..9537378f0 100644 --- a/test-frontend-helpers.js +++ b/test-frontend-helpers.js @@ -2242,6 +2242,12 @@ console.log('\n=== app.js: computeBreakdownRanges ==='); assert.strictEqual(ctx.senderPathHashSize('1540DEADBEEF'), 2); assert.strictEqual(ctx.senderPathHashSize('1580DEADBEEF'), 3); assert.strictEqual(ctx.senderPathHashSize('1600DEADBEEF'), null); + // #322 (1): the direct zero-hop marker is also valid on TRANSPORT_DIRECT + // (route 3), where the path byte sits at offset 5 behind the transport + // codes. Header 0x17 = route 3, 4 transport bytes (aabbccdd), path byte + // 0x00. The shared width helper must treat route 3 like route 2 → null, + // killing the `(route === 2 || route === 3)` → `(route === 2)` mutant here. + assert.strictEqual(ctx.senderPathHashSize('17aabbccdd00DEADBEEF'), null); assert.strictEqual(ctx.senderPathHashSize('141122334440DEADBEEF'), 2); assert.strictEqual(ctx.senderPathHashSize('2540DEADBEEF'), null); assert.strictEqual(ctx.senderPathHashSize('15C0DEADBEEF'), null); @@ -6148,6 +6154,9 @@ console.log('\n=== packets.js: buildFieldTable transport offsets (#765) ==='); loadInCtx(hashHelperCtx, 'public/roles.js'); loadInCtx(hashHelperCtx, 'public/app.js'); ftCtx.senderPathHashSize = hashHelperCtx.senderPathHashSize; + // #322 (1): buildFieldTable's Path Length row now reads its width through the + // shared pathHashSizeFromByte() helper, so the sandbox must expose it too. + ftCtx.pathHashSizeFromByte = hashHelperCtx.pathHashSizeFromByte; loadInCtx(ftCtx, 'public/packets.js'); const { buildFieldTable, fieldRow } = ftCtx.window._packetsTestAPI; @@ -6249,6 +6258,8 @@ console.log('\n=== packets.js: buildFieldTable hop count from path_len (#844) == loadInCtx(secondHashHelperCtx, 'public/roles.js'); loadInCtx(secondHashHelperCtx, 'public/app.js'); ftCtx.senderPathHashSize = secondHashHelperCtx.senderPathHashSize; + // #322 (1): buildFieldTable now reads its width via the shared helper. + ftCtx.pathHashSizeFromByte = secondHashHelperCtx.pathHashSizeFromByte; loadInCtx(ftCtx, 'public/packets.js'); const { buildFieldTable } = ftCtx.window._packetsTestAPI; diff --git a/test-issue-322-comment-guards.js b/test-issue-322-comment-guards.js new file mode 100644 index 000000000..f0f53386a --- /dev/null +++ b/test-issue-322-comment-guards.js @@ -0,0 +1,58 @@ +/** + * #322 comment fixes (points 3 and 4), follow-ups to #313 (#282). These are + * documentation corrections with no runtime surface, so the regression guard is + * on the source text itself: each assertion is red against the wrong wording and + * green once corrected, and re-introducing the old wording (the mutant) turns it + * red again. The behaviours the comments describe are exercised elsewhere: + * - the pktEsc once-per-visit leak (point 3) by test-issue-282-pktesc-listener.js; + * - normalizeObservedPathHashSizes()'s real callers (point 4) by + * test-channels-observed-path-hash-size.js (normalize/union) and the + * dedup/merge paths in channels.js. + * + * Usage: node test-issue-322-comment-guards.js + */ +'use strict'; +const fs = require('fs'); +const assert = require('assert'); + +let passed = 0, failed = 0; +function test(name, fn) { + try { fn(); passed++; console.log(' ✅ ' + name); } + catch (e) { failed++; console.log(' ❌ ' + name + ': ' + e.message); } +} + +const read = (f) => fs.readFileSync(f, 'utf8'); +// Collapse whitespace after dropping per-line comment margins (JSDoc `*` and +// `//`) so a phrase that wraps across comment lines matches as one string. +const squash = (s) => s.replace(/^\s*(?:\/\/|\*)\s?/gm, '').replace(/\s+/g, ' ').trim(); + +console.log('\n=== #322 comment fixes ==='); + +// --- Point 3: the pktEsc leak header ----------------------------------------- +// renderLeft() also runs on filter/region changes, but those later calls return +// at the `filtersBuilt` guard before the addEventListener, so the closure was +// added once per VISIT, not per filter/region change. The old header claimed +// otherwise. +test('point 3: the pktEsc header does not claim the leak stacked on every filter/region change', () => { + const header = read('test-issue-282-pktesc-listener.js'); + const h = squash(header.slice(0, header.indexOf('*/'))); + assert(!h.includes('runs on every visit to #/packets (and on every filter/region change within a visit)'), + 'the header still claims renderLeft stacks a listener on every filter/region change within a visit'); + assert(h.includes('filtersBuilt'), + 'the header should explain that later renders return at the filtersBuilt guard before the addEventListener'); +}); + +// --- Point 4: normalizeObservedPathHashSizes()'s real callers ---------------- +// renderSenderPathHashBadge() reads message.senderPathHashSize directly; it does +// NOT call normalizeObservedPathHashSizes(). The old comment said the sender +// badge used it. +test('point 4: channels.js does not say the sender badge uses normalizeObservedPathHashSizes()', () => { + const src = squash(read('public/channels.js')); + assert(!src.includes('the union/merge path and the sender badge still use it'), + 'the comment still claims the sender badge uses normalizeObservedPathHashSizes()'); + assert(src.includes('renderSenderPathHashBadge does not'), + 'the corrected comment should state renderSenderPathHashBadge does not call it'); +}); + +console.log(`\n${passed} passed, ${failed} failed`); +process.exit(failed === 0 ? 0 : 1); diff --git a/test-packets.js b/test-packets.js index 495b95920..33ed65546 100644 --- a/test-packets.js +++ b/test-packets.js @@ -1057,6 +1057,23 @@ console.log('\n=== packets.js: buildFieldTable ==='); 'direct zero-hop marker description drifted, got: ' + result); }); + // #322 (2): the zero-hop 0x00 marker is also valid on TRANSPORT_DIRECT + // (route 3, firmware/src/Packet.h isRouteDirect()), where the path-length byte + // sits at offset 5 behind the transport codes. Nothing covered route 3, so the + // direct-marker mutant `(route === 2 || route === 3)` → `(route === 2)` + // survived: it would relabel this 0x00 as hash_size=1. This case kills it. + test('buildFieldTable keeps the direct zero-hop marker on TRANSPORT_DIRECT (route 3, byte 5)', () => { + // header 0x17 = route 3 (TRANSPORT_DIRECT), path-type 5 (hops). 4 transport + // code bytes (aabbccdd), then path byte 0x00 at offset 5 = sendZeroHop's + // marker. route 3 must be treated like route 2: no encoded hash size. + const pkt = { raw_hex: '17aabbccdd00', route_type: 3, payload_type: 5 }; + const result = api.buildFieldTable(pkt, {}, [], []); + assert(result.includes('hash_count=0 (no encoded hash size)'), + 'TRANSPORT_DIRECT zero-hop marker must read as no encoded hash size, got: ' + result); + assert(!/hash_size=\d/.test(result), + 'the 0x00 direct marker must not be read as a hash width on route 3, got: ' + result); + }); + test('buildFieldTable does not read a hash width out of TRACE SNR bytes', () => { // header 0x25 = payload 9 (TRACE), route 1. Its path bytes are SNR // readings (internal/packetpath/route.go PathBytesAreHops), not hops. From 40f36f8aa2d1538e8ca39caea982a06ba9d5290d Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 16:23:04 +0200 Subject: [PATCH 2/2] fix(packets): one path-hash width helper for the hex breakdown Follow-ups to #313 (#282). - One implementation of the path-hash width rules: pathHashSizeFromByte() in app.js (TRACE -> null, 0b11 -> null, 0x00 zero-hop marker on a direct route 2/3 -> null, else (byte >> 6) + 1). senderPathHashSize() and buildFieldTable's Path Length row both call it, each still reading its own path byte at its own offset (header-derived vs pkt.route_type), so the rules cannot drift. Verified against firmware/src/Packet.h and firmware/docs/packet_format.md. - channels.js: the #282 (8) comment no longer claims the sender badge uses normalizeObservedPathHashSizes(); renderSenderPathHashBadge reads message.senderPathHashSize directly. Lists the real callers. - test-issue-282-pktesc-listener.js: the header no longer says the leak stacked per filter/region change; renderLeft() returns at the filtersBuilt guard, so the listener was added once per visit. - .eslintrc.json: register the new cross-file global; test plumbing lifts it. Co-Authored-By: Claude Opus 4.8 --- .eslintrc.json | 1 + public/app.js | 32 +++++++++++++++++++++--- public/channels.js | 6 +++-- public/packets.js | 28 ++++++++++----------- test-channels-observed-path-hash-size.js | 2 +- test-issue-282-pktesc-listener.js | 18 ++++++++----- 6 files changed, 59 insertions(+), 28 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index a43d8b3f3..655cbcbd0 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -257,6 +257,7 @@ "pad3": "readonly", "pages": "readonly", "parseViewportHash": "readonly", + "pathHashSizeFromByte": "readonly", "payloadTypeColor": "readonly", "payloadTypeName": "readonly", "process": "readonly", diff --git a/public/app.js b/public/app.js index e1dafd7a5..5a0519b4b 100644 --- a/public/app.js +++ b/public/app.js @@ -12,19 +12,43 @@ function payloadTypeColor(n) { return PAYLOAD_COLORS[n] || 'unknown'; } function isTransportRoute(rt) { return rt === 0 || rt === 3; } /** Byte offset of path_len in raw_hex: 5 for transport routes (4 bytes of next/last hop codes precede it), 1 otherwise. */ function getPathLenOffset(routeType) { return isTransportRoute(routeType) ? 5 : 1; } +/** + * #322 (1): the ONE implementation of the path-hash width rules. Given a + * path-length byte, the route it belongs to, and the frame's header byte, + * return the sender-selected hash width (1-3 bytes) or null when the byte + * encodes no width. Both senderPathHashSize() below and the packet-detail Path + * Length row (public/packets.js buildFieldTable) call this, so the rules live + * in one place (AGENTS.md: one implementation) and cannot drift. Each caller + * still reads its own path byte at its own offset -- senderPathHashSize derives + * route+offset from the header byte, the Path Length row from pkt.route_type -- + * so there is one offset source per caller; only the width semantics are here. + * The rules are the firmware's (firmware/src/Packet.h getPathHashSize()/ + * isRouteDirect(), firmware/docs/packet_format.md "path_length"): + * - null for TRACE (headerByte path-type 9): those path bytes are per-hop SNR, + * not a hash width (internal/packetpath/route.go PathBytesAreHops); + * - null for a 0b11 width field: a 4-byte width is reserved/invalid, the + * backend evidence model only knows 1/2/3 (cmd/server/observed_path_hash_sizes.go); + * - null for the 0x00 zero-hop marker on a direct route (2 or 3, Packet.h + * isRouteDirect(), sendZeroHop()); + * - otherwise (pathByte >> 6) + 1. + */ +function pathHashSizeFromByte(pathByte, routeType, headerByte) { + if (typeof pathByte !== 'number' || isNaN(pathByte)) return null; + if (typeof headerByte === 'number' && !isNaN(headerByte) && ((headerByte >> 2) & 0x0F) === 9) return null; // TRACE path bytes are SNR + if (pathByte === 0 && (routeType === 2 || routeType === 3)) return null; // direct zero-hop marker + const size = (pathByte >> 6) + 1; + return size <= 3 ? size : null; +} /** Sender-selected path-hash width in this frame, or null when not encoded. */ function senderPathHashSize(rawHex) { if (typeof rawHex !== 'string' || !/^[0-9a-f]{2}/i.test(rawHex)) return null; const header = parseInt(rawHex.slice(0, 2), 16); - if (((header >> 2) & 0x0F) === 9) return null; // TRACE path bytes are SNR const route = header & 0x03; const offset = getPathLenOffset(route) * 2; const pathHex = rawHex.slice(offset, offset + 2); if (!/^[0-9a-f]{2}$/i.test(pathHex)) return null; const pathByte = parseInt(pathHex, 16); - if (pathByte === 0 && (route === 2 || route === 3)) return null; - const size = (pathByte >> 6) + 1; - return size <= 3 ? size : null; + return pathHashSizeFromByte(pathByte, route, header); } /** * scopeName is optional (callers that don't pass it get the original diff --git a/public/channels.js b/public/channels.js index 660dee042..8acb72835 100644 --- a/public/channels.js +++ b/public/channels.js @@ -59,8 +59,10 @@ // live here, but the only remaining reader was the test export -- no shipped // code rendered the "Observed path hash" badge (renderSenderPathHashBadge is // the one wired into renderMessages). Removed with its test so a dead helper - // cannot drift. normalizeObservedPathHashSizes() stays: the union/merge path - // and the sender badge still use it. + // cannot drift. normalizeObservedPathHashSizes() stays: the union/merge path, + // the cached-message merge, the packet -> message mapping and the dedup key + // still use it. renderSenderPathHashBadge does not -- it reads + // message.senderPathHashSize directly, not the observed-path list. // The header records the sender's choice even before a flood has relayed. // Observation-path evidence is retained internally, but is not the label. diff --git a/public/packets.js b/public/packets.js index ed073ec3e..7d72ea4d5 100644 --- a/public/packets.js +++ b/public/packets.js @@ -3924,21 +3924,19 @@ const headerByte = parseInt(buf.slice(0, 2), 16); const pathBytesAreHops = !isNaN(headerByte) && ((headerByte >> 2) & 0x0F) !== 9; // #282 (7): derive the encoded hash size from the SAME path-length byte this - // row shows (pathByte0 at offset `off`, taken from pkt.route_type) rather - // than calling senderPathHashSize(buf), which independently re-derives the - // offset from the raw_hex header byte. For a well-formed frame the two - // offsets agree, but one source means the printed byte and its hash_size - // label can never describe different bytes -- including a transport route - // (path length at byte 5) whose stored route_type and on-wire header route - // bits might disagree. The width semantics are senderPathHashSize's own: - // null for a non-hop path (TRACE carries SNR), for a 0b11 width field (there - // is no 4-byte width -- the backend evidence model only knows 1/2/3, see - // observed_path_hash_sizes.go), and for sendZeroHop's 0x00 direct marker. - let encodedHashSize = null; - if (pathBytesAreHops && !isNaN(pathByte0) && (pathByte0 >> 6) !== 3 && - !(pathByte0 === 0 && (pkt.route_type === 2 || pkt.route_type === 3))) { - encodedHashSize = (pathByte0 >> 6) + 1; - } + // row shows -- pathByte0 at offset `off`, taken from pkt.route_type. #322 + // (1): the width rules themselves live in one place, pathHashSizeFromByte() + // in app.js, shared with senderPathHashSize(); this caller just hands it the + // byte it already read plus pkt.route_type. For a well-formed frame the two + // callers' offsets agree, but one rule means the printed byte and its + // hash_size label can never describe different bytes -- including a transport + // route (path length at byte 5) whose stored route_type and on-wire header + // route bits might disagree. The helper returns null for a non-hop path + // (TRACE carries SNR), a 0b11 width field (no 4-byte width -- the backend + // evidence model only knows 1/2/3, see observed_path_hash_sizes.go), and + // sendZeroHop's 0x00 direct marker; the branches below only pick the wording + // for each null case. + const encodedHashSize = pathHashSizeFromByte(pathByte0, pkt.route_type, headerByte); let pathDescription; if (encodedHashSize != null) { pathDescription = `hash_size=${encodedHashSize} byte${encodedHashSize !== 1 ? 's' : ''}, hash_count=${hashCountVal}`; diff --git a/test-channels-observed-path-hash-size.js b/test-channels-observed-path-hash-size.js index 5440da316..96cd03e76 100644 --- a/test-channels-observed-path-hash-size.js +++ b/test-channels-observed-path-hash-size.js @@ -107,7 +107,7 @@ const ctx = { // '59C0' -> 3, a width the real helper reports as unknown), so the merge // assertions below would have passed against a contract nothing ships. senderPathHashSize: loadAppHelper('senderPathHashSize', - ['isTransportRoute', 'getPathLenOffset', 'senderPathHashSize']), + ['isTransportRoute', 'getPathLenOffset', 'pathHashSizeFromByte', 'senderPathHashSize']), }; vm.createContext(ctx); // Expose the cache fetch helper only inside this VM so the regression can diff --git a/test-issue-282-pktesc-listener.js b/test-issue-282-pktesc-listener.js index fbca0e90d..cb8b13b0e 100644 --- a/test-issue-282-pktesc-listener.js +++ b/test-issue-282-pktesc-listener.js @@ -3,9 +3,11 @@ * not leak. * * public/packets.js registered a fresh `pktEsc` closure on `document` inside - * renderLeft(), and renderLeft() runs on every visit to #/packets (and on every - * filter/region change within a visit). The closure was never removed, so each - * entry stacked one more live `keydown` listener on `document` -- the same leak + * renderLeft(). #322 (3): renderLeft() also runs on every filter/region change + * within a visit, but those later calls return at the `filtersBuilt` guard + * before reaching the addEventListener, so the closure was added once per VISIT + * to #/packets, not per filter/region change. It was never removed, so each + * visit stacked one more live `keydown` listener on `document` -- the same leak * class as nodes.js' #259 nodesEsc/nodesPanelEsc. destroy() never took it off. * * The fix makes `_pktEsc` a stable module-level reference: a repeat @@ -18,7 +20,8 @@ * router's init/destroy cycle for several #/packets visits and asserts: * - a single visit leaves exactly one document keydown listener; * - entering and leaving #/packets several times does not stack listeners; - * - a second render inside the same visit does not add a second listener; + * - a re-render inside the same visit (a filter/region change) returns at the + * `filtersBuilt` guard and so adds no second listener; * - destroy() leaves no document keydown listener behind; * - the listener still works on a page opened after a destroy. * @@ -218,11 +221,14 @@ test('entering and leaving #/packets several times does not stack listeners', as assert.strictEqual(n, 1, 'at most one listener after four visits, got ' + n); }); -test('a second render inside the same visit does not add a second listener', async () => { +test('a re-render inside the same visit returns at the filtersBuilt guard and adds no second listener', async () => { const s = loadPackets(); s.page.init(s.app, null); await settle(); - s.regionChange(); // a region change re-runs loadPackets() -> renderLeft() + // A region change re-runs loadPackets() -> renderLeft(), but renderLeft() + // returns at the `filtersBuilt` guard before the addEventListener, so no + // second listener is added even on master. This pins that early-return path. + s.regionChange(); await settle(); assert.deepStrictEqual(s.errors, [], 'both renders ran without errors'); const n = s.keydownListeners().length;