fix(packets): one path-hash width helper for the hex breakdown (#322)

.eslintrc.json

@@ -257,6 +257,7 @@
     "pad3": "readonly",
     "pages": "readonly",
     "parseViewportHash": "readonly",
+    "pathHashSizeFromByte": "readonly",
     "payloadTypeColor": "readonly",
     "payloadTypeName": "readonly",
     "process": "readonly",

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

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.

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}`;

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

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

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;
 

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;

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);

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.