test: close test gaps for #123 decrypted status and #131 WS handler runtime

Gap 1 (#123): Add 3 decoder tests for GRP_TXT decrypted status path. Mock ChannelCrypto via require.cache to simulate successful decryption. Tests cover: sender+message formatting, no-sender fallback, multi-key iteration with first-match-wins semantics. Gap 2 (#131): Rewrite 5 src.includes() string-match tests as runtime vm.createContext tests. New makeNodesWsSandbox() helper with controllable setTimeout, mock DOM, tracked API/cache calls, and real debouncedOnWS. Tests verify: ADVERT triggers refresh, non-ADVERT ignored, debounce collapses multiple ADVERTs, cache reset forces re-fetch, scroll/selection preserved during WS-triggered refresh. Decoder: 58 -> 61 tests. Frontend helpers: 87 (5 replaced, not added). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2026-03-30 14:45:52 +00:00 · 2026-03-27 08:47:52 -07:00
parent 3cd6cb98fa
commit 1137dd1c08
3 changed files with 259 additions and 50 deletions
--- a/.squad/agents/bishop/history.md
+++ b/.squad/agents/bishop/history.md
@@ -55,3 +55,15 @@ User: User
  - OPTIMIZATION OPPORTUNITIES: Replace networkidle→domcontentloaded (same fix as E2E tests), replace waitForTimeout with event-driven waits, reduce/batch page navigations, parallelize independent page exercises
 - REGRESSION TESTS ADDED 2026-03-27: Memory optimization (observation deduplication). 8 new tests in test-packet-store.js under "=== Observation deduplication (transmission_id refs) ===" section. Tests verify: (1) observations don't duplicate raw_hex/decoded_json, (2) transmission fields accessible via store.byTxId.get(obs.transmission_id), (3) query() and all() still return transmission fields for backward compat, (4) multiple observations share one transmission_id, (5) getSiblings works after dedup, (6) queryGrouped returns transmission fields, (7) memory estimate reflects dedup savings. 4 tests fail pre-fix (expected — Hicks hasn't applied changes yet), 4 pass (backward compat). Pattern: use hasOwnProperty() to distinguish own vs inherited/absent fields.
 - REVIEW 2026-03-27: Hicks RAM fix (observation dedup). REJECTED. Tests pass (42 packet-store + 204 route), but 5 server.js consumers access `.hash`, `.raw_hex`, `.decoded_json`, `.payload_type` on lean observations from `byObserver.get()` or `tx.observations` without enrichment. Broken endpoints: (1) `/api/nodes/bulk-health` line 1141 `o.hash` undefined, (2) `/api/nodes/network-status` line 1220 `o.hash` undefined, (3) `/api/analytics/signal` lines 1298+1306 `p.hash`/`p.raw_hex` undefined, (4) `/api/observers/:id/analytics` lines 2320+2329+2361 `p.payload_type`/`p.decoded_json` undefined + lean objects sent to client as recentPackets, (5) `/api/analytics/subpaths` line 2711 `o.hash` undefined. All are regional filtering or analytics code paths that use `byObserver` directly. Fix: either enrich at these call sites or store `hash` on observations (it's small). The enrichment pattern works for `getById()`, `getSiblings()`, and `/api/packets/:id` but was not applied to the 5 other consumers. Route tests pass because they don't assert on these specific field values in analytics responses.
+- BATCH REVIEW 2026-03-27: Reviewed 6 issue fixes pushed without sign-off. Full suite: 971 tests, 0 failures across 11 test files. Cache busters uniform (v=1774625000). Verdicts:
+  - #133 (phantom nodes): ✅ APPROVED. 12 assertions on removePhantomNodes, real db.js code, edge cases (idempotency, real node preserved, stats filtering).
+  - #123 (channel hash): ⚠️ APPROVED WITH NOTES. 6 new decoder tests cover channelHashHex (zero-padding) and decryptionStatus (no_key ×3, decryption_failed). Missing: `decrypted` status untested (needs valid crypto key), frontend rendering of "Ch 0xXX (no key)" untested.
+  - #126 (offline node on map): ✅ APPROVED. 3 regression tests: ambiguous prefix→null, unique prefix→resolves, dead node stays dead. Caching verified. Excellent quality.
+  - #130 (disappearing nodes): ✅ APPROVED. 8 pruneStaleNodes tests cover dim/restore/remove for API vs WS nodes. Real live.js via vm.createContext.
+  - #131 (auto-updating nodes): ⚠️ APPROVED WITH NOTES. 8 solid isAdvertMessage tests (real code). BUT 5 WS handler tests are source-string-match checks (`src.includes('loadNodes(true)')`) — these verify code exists but not that it works at runtime. No runtime test for debounce batching behavior.
+  - #129 (observer comparison): ✅ APPROVED. 11 comprehensive tests for comparePacketSets — all edge cases, performance (10K hashes <500ms), mathematical invariant. Real compare.js via vm.createContext.
+  - NOTES FOR IMPROVEMENT: (1) #131 debounce behavior should get a runtime test via vm.createContext, not string checks. (2) #123 could benefit from a `decrypted` status test if crypto mocking is feasible. Neither is blocking.
+- TEST GAP FIX 2026-03-27: Closed both noted gaps from batch review:
+  - #123 (channel hash decryption `decrypted` status): 3 new tests in test-decoder.js. Used require.cache mocking to swap ChannelCrypto module with mock that returns `{success:true, data:{...}}`. Tests cover: (1) decrypted status with sender+message (text formatted as "Sender: message"), (2) decrypted without sender (text is just message), (3) multiple keys tried, first match wins (verifies iteration order + call count). All verify channelHashHex, type='CHAN', channel name, sender, timestamp, flags. require.cache is restored in finally block.
+  - #131 (WS handler runtime tests): Rewrote 5 `src.includes()` string-match tests to use vm.createContext with runtime execution. Created `makeNodesWsSandbox()` helper that provides controllable setTimeout (timer queue), mock DOM, tracked api/invalidateApiCache calls, and real `debouncedOnWS` logic. Tests run actual nodes.js init() and verify: (1) ADVERT triggers refresh with 5s debounce, (2) non-ADVERT doesn't trigger refresh, (3) debounce collapses 3 ADVERTs into 1 API call, (4) _allNodes cache reset forces re-fetch, (5) scroll/selection preserved (panel innerHTML + scrollTop untouched by WS handler). Total: 87 frontend helper tests (same count — 5 replaced, not added), 61 decoder tests (+3).
+  - Technique learned: require.cache mocking is effective for testing code paths that depend on external modules (like ChannelCrypto). Store original, replace exports, restore in finally. Controllable setTimeout (capturing callbacks in array, firing manually) enables testing debounce logic without real timers.
--- a/test-decoder.js
+++ b/test-decoder.js
@@ -247,6 +247,100 @@ test('GRP_TXT decryptionStatus is no_key when encrypted data too short', () => {
  assert.strictEqual(p.payload.decryptionStatus, 'no_key');
 });

+test('GRP_TXT decryptionStatus is decrypted when key matches', () => {
+  // Mock the ChannelCrypto module to simulate successful decryption
+  const cryptoPath = require.resolve('@michaelhart/meshcore-decoder/dist/crypto/channel-crypto');
+  const originalModule = require.cache[cryptoPath];
+  require.cache[cryptoPath] = {
+    id: cryptoPath,
+    exports: {
+      ChannelCrypto: {
+        decryptGroupTextMessage: () => ({
+          success: true,
+          data: { sender: 'TestUser', message: 'Hello world', timestamp: 1700000000, flags: 0 },
+        }),
+      },
+    },
+  };
+  try {
+    const hex = '1500' + 'FF' + 'AABB' + 'CCDDEE112233';
+    const p = decodePacket(hex, { '#general': 'aabbccddaabbccddaabbccddaabbccdd' });
+    assert.strictEqual(p.payload.decryptionStatus, 'decrypted');
+    assert.strictEqual(p.payload.type, 'CHAN');
+    assert.strictEqual(p.payload.channelHashHex, 'FF');
+    assert.strictEqual(p.payload.channel, '#general');
+    assert.strictEqual(p.payload.sender, 'TestUser');
+    assert.strictEqual(p.payload.text, 'TestUser: Hello world');
+    assert.strictEqual(p.payload.sender_timestamp, 1700000000);
+    assert.strictEqual(p.payload.flags, 0);
+    assert.strictEqual(p.payload.channelHash, 0xFF);
+  } finally {
+    if (originalModule) require.cache[cryptoPath] = originalModule;
+    else delete require.cache[cryptoPath];
+  }
+});
+
+test('GRP_TXT decrypted without sender formats text correctly', () => {
+  const cryptoPath = require.resolve('@michaelhart/meshcore-decoder/dist/crypto/channel-crypto');
+  const originalModule = require.cache[cryptoPath];
+  require.cache[cryptoPath] = {
+    id: cryptoPath,
+    exports: {
+      ChannelCrypto: {
+        decryptGroupTextMessage: () => ({
+          success: true,
+          data: { sender: null, message: 'Broadcast msg', timestamp: 1700000001, flags: 1 },
+        }),
+      },
+    },
+  };
+  try {
+    const hex = '1500' + '0A' + 'AABB' + 'CCDDEE112233';
+    const p = decodePacket(hex, { '#alerts': 'deadbeefdeadbeefdeadbeefdeadbeef' });
+    assert.strictEqual(p.payload.decryptionStatus, 'decrypted');
+    assert.strictEqual(p.payload.sender, null);
+    assert.strictEqual(p.payload.text, 'Broadcast msg');
+    assert.strictEqual(p.payload.channelHashHex, '0A');
+  } finally {
+    if (originalModule) require.cache[cryptoPath] = originalModule;
+    else delete require.cache[cryptoPath];
+  }
+});
+
+test('GRP_TXT decrypted tries multiple keys, first match wins', () => {
+  const cryptoPath = require.resolve('@michaelhart/meshcore-decoder/dist/crypto/channel-crypto');
+  const originalModule = require.cache[cryptoPath];
+  let callCount = 0;
+  require.cache[cryptoPath] = {
+    id: cryptoPath,
+    exports: {
+      ChannelCrypto: {
+        decryptGroupTextMessage: (ciphertext, mac, key) => {
+          callCount++;
+          if (key === 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb') {
+            return { success: true, data: { sender: 'Bob', message: 'Found it', timestamp: 0, flags: 0 } };
+          }
+          return { success: false };
+        },
+      },
+    },
+  };
+  try {
+    const hex = '1500' + 'FF' + 'AABB' + 'CCDDEE112233';
+    const p = decodePacket(hex, {
+      '#wrong': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa',
+      '#right': 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb',
+    });
+    assert.strictEqual(p.payload.decryptionStatus, 'decrypted');
+    assert.strictEqual(p.payload.channel, '#right');
+    assert.strictEqual(p.payload.sender, 'Bob');
+    assert.strictEqual(callCount, 2);
+  } finally {
+    if (originalModule) require.cache[cryptoPath] = originalModule;
+    else delete require.cache[cryptoPath];
+  }
+});
+
 console.log('\n=== TXT_MSG payload ===');
 test('TXT_MSG decode', () => {
  // payloadType=2 → (2<<2)|1 = 0x09
--- a/test-frontend-helpers.js
+++ b/test-frontend-helpers.js
@@ -834,70 +834,173 @@ console.log('\n=== nodes.js: isAdvertMessage ===');
  });
 }

-console.log('\n=== nodes.js: loadNodes refreshOnly =====');
+console.log('\n=== nodes.js: WS handler runtime behavior ===');
 {
-  // Verify that loadNodes(true) calls renderRows instead of renderLeft
-  const ctx = makeSandbox();
-  ctx.ROLE_COLORS = { repeater: '#22c55e', room: '#6366f1', companion: '#3b82f6', sensor: '#f59e0b' };
-  ctx.ROLE_STYLE = {};
-  ctx.TYPE_COLORS = {};
-  ctx.getNodeStatus = () => 'active';
-  ctx.getHealthThresholds = () => ({ staleMs: 600000, degradedMs: 1800000, silentMs: 86400000 });
-  ctx.timeAgo = () => '1m ago';
-  ctx.truncate = (s) => s;
-  ctx.escapeHtml = (s) => String(s || '');
-  ctx.payloadTypeName = () => 'Advert';
-  ctx.payloadTypeColor = () => 'advert';
-  ctx.debouncedOnWS = () => null;
-  ctx.onWS = () => {};
-  ctx.offWS = () => {};
-  ctx.debounce = (fn) => fn;
-  ctx.invalidateApiCache = () => {};
-  ctx.CLIENT_TTL = { nodeList: 90000, nodeDetail: 240000, nodeHealth: 240000 };
-  ctx.initTabBar = () => {};
-  ctx.getFavorites = () => [];
-  ctx.favStar = () => '';
-  ctx.bindFavStars = () => {};
-  ctx.makeColumnsResizable = () => {};
-  ctx.Set = Set;
-  ctx.RegionFilter = { init: () => {}, onChange: () => () => {}, getRegionParam: () => '' };
+  // Runtime tests for the auto-updating WS handler (replaces src.includes string checks).
+  // Uses controllable setTimeout + mock DOM + real nodes.js code via vm.createContext.

-  let renderLeftCalls = 0;
-  let renderRowsCalls = 0;
-  let initCaptured = null;
+  function makeNodesWsSandbox() {
+    const ctx = makeSandbox();
+    // Controllable timer queue
+    const timers = [];
+    let nextTimerId = 1;
+    ctx.setTimeout = (fn, ms) => { const id = nextTimerId++; timers.push({ fn, ms, id }); return id; };
+    ctx.clearTimeout = (targetId) => { const idx = timers.findIndex(t => t.id === targetId); if (idx >= 0) timers.splice(idx, 1); };

-  ctx.api = () => Promise.resolve({ nodes: [{ public_key: 'abc123def456ghij', name: 'TestNode', role: 'repeater' }], counts: { repeaters: 1 } });
-  ctx.registerPage = (name, handlers) => { initCaptured = handlers; };
+    // DOM elements mock — getElementById returns tracked mock elements
+    const domElements = {};
+    function getEl(id) {
+      if (!domElements[id]) {
+        domElements[id] = {
+          id, innerHTML: '', textContent: '', value: '', scrollTop: 0,
+          style: {}, dataset: {},
+          classList: { add() {}, remove() {}, toggle() {}, contains() { return false; } },
+          addEventListener() {},
+          querySelectorAll() { return []; },
+          querySelector() { return null; },
+          getAttribute() { return null; },
+        };
+      }
+      return domElements[id];
+    }
+    ctx.document.getElementById = getEl;
+    ctx.document.querySelectorAll = () => [];
+    ctx.document.addEventListener = () => {};
+    ctx.document.removeEventListener = () => {};

-  // Load nodes.js — captures the init/destroy via registerPage
-  loadInCtx(ctx, 'public/nodes.js');
+    // Globals nodes.js depends on
+    ctx.ROLE_COLORS = { repeater: '#22c55e', room: '#6366f1', companion: '#3b82f6', sensor: '#f59e0b' };
+    ctx.ROLE_STYLE = {};
+    ctx.TYPE_COLORS = {};
+    ctx.getNodeStatus = () => 'active';
+    ctx.getHealthThresholds = () => ({ staleMs: 600000, degradedMs: 1800000, silentMs: 86400000 });
+    ctx.timeAgo = () => '1m ago';
+    ctx.truncate = (s) => s;
+    ctx.escapeHtml = (s) => String(s || '');
+    ctx.payloadTypeName = () => 'Advert';
+    ctx.payloadTypeColor = () => 'advert';
+    ctx.debounce = (fn) => fn;
+    ctx.initTabBar = () => {};
+    ctx.getFavorites = () => [];
+    ctx.favStar = () => '';
+    ctx.bindFavStars = () => {};
+    ctx.makeColumnsResizable = () => {};
+    ctx.Set = Set;
+    ctx.CLIENT_TTL = { nodeList: 90000, nodeDetail: 240000, nodeHealth: 240000 };
+    ctx.RegionFilter = { init() {}, onChange() { return () => {}; }, getRegionParam() { return ''; }, offChange() {} };

-  // Now we need to call init, but it requires DOM.
-  // Instead, test the loadNodes function behavior via the WS handler pattern.
-  // The key test: debouncedOnWS callback should call loadNodes(true) for adverts.
-  // We already tested isAdvertMessage above. Here we verify the refreshOnly param
-  // is passed correctly by checking it was wired in the source.
-  const src = fs.readFileSync('public/nodes.js', 'utf8');
+    // Track API calls and cache invalidation
+    let apiCallCount = 0;
+    const invalidatedPaths = [];
+    ctx.api = () => { apiCallCount++; return Promise.resolve({ nodes: [{ public_key: 'abc123def456ghij', name: 'TestNode', role: 'repeater', advert_count: 1 }], counts: { repeaters: 1 } }); };
+    ctx.invalidateApiCache = (path) => { invalidatedPaths.push(path); };

-  test('WS handler calls loadNodes with refreshOnly=true', () => {
-    assert.ok(src.includes('loadNodes(true)'), 'WS handler should call loadNodes(true) for refresh-only updates');
+    // WS listener system (real debouncedOnWS from app.js, using our controllable setTimeout)
+    let wsListeners = [];
+    ctx.onWS = (fn) => { wsListeners.push(fn); };
+    ctx.offWS = (fn) => { wsListeners = wsListeners.filter(f => f !== fn); };
+    ctx.debouncedOnWS = function (fn, ms) {
+      if (typeof ms === 'undefined') ms = 250;
+      let pending = [];
+      let timer = null;
+      function handler(msg) {
+        pending.push(msg);
+        if (!timer) {
+          timer = ctx.setTimeout(function () {
+            const batch = pending;
+            pending = [];
+            timer = null;
+            fn(batch);
+          }, ms);
+        }
+      }
+      wsListeners.push(handler);
+      return handler;
+    };
+
+    // Capture registerPage to get init/destroy
+    let pageMod = null;
+    ctx.registerPage = (name, handlers) => { pageMod = handlers; };
+
+    loadInCtx(ctx, 'public/nodes.js');
+
+    // Create a mock app element and call init()
+    const appEl = getEl('page');
+    pageMod.init(appEl);
+
+    // Reset counters after init's own loadNodes() call
+    apiCallCount = 0;
+    invalidatedPaths.length = 0;
+
+    return {
+      ctx, timers, wsListeners, domElements,
+      getApiCalls: () => apiCallCount,
+      getInvalidated: () => [...invalidatedPaths],
+      resetCounters() { apiCallCount = 0; invalidatedPaths.length = 0; },
+      fireTimers() { const fns = timers.splice(0).map(t => t.fn); fns.forEach(fn => fn()); },
+      sendWS(msg) { wsListeners.forEach(fn => fn(msg)); },
+    };
+  }
+
+  test('ADVERT packet triggers node list refresh via WS handler', () => {
+    const env = makeNodesWsSandbox();
+    env.sendWS({ type: 'packet', data: { packet: { payload_type: 4 } } });
+    assert.strictEqual(env.timers.length, 1, 'debounce timer should be queued');
+    assert.strictEqual(env.timers[0].ms, 5000, 'debounce should be 5000ms');
+    env.fireTimers();
+    assert.ok(env.getInvalidated().includes('/nodes'), 'should invalidate /nodes cache');
+    assert.ok(env.getApiCalls() > 0, 'should call api() to re-fetch nodes');
  });

-  test('WS handler uses 5-second debounce', () => {
-    assert.ok(src.includes('}, 5000)'), 'debouncedOnWS should use 5000ms debounce');
+  test('non-ADVERT packet does NOT trigger refresh', () => {
+    const env = makeNodesWsSandbox();
+    env.sendWS({ type: 'packet', data: { packet: { payload_type: 2 } } });
+    env.fireTimers();
+    assert.strictEqual(env.getApiCalls(), 0, 'api should not be called for non-ADVERT');
+    assert.deepStrictEqual(env.getInvalidated(), [], 'no cache invalidation for non-ADVERT');
  });

-  test('WS handler invalidates API cache before refresh', () => {
-    assert.ok(src.includes("invalidateApiCache('/nodes')"), 'Should invalidate /nodes cache');
+  test('debounce collapses multiple ADVERTs within 5s into one refresh', () => {
+    const env = makeNodesWsSandbox();
+    env.sendWS({ type: 'packet', data: { packet: { payload_type: 4 } } });
+    env.sendWS({ type: 'packet', data: { packet: { payload_type: 4 } } });
+    env.sendWS({ type: 'packet', data: { packet: { payload_type: 4 } } });
+    assert.strictEqual(env.timers.length, 1, 'only one debounce timer despite 3 messages');
+    env.fireTimers();
+    assert.ok(env.getApiCalls() > 0, 'api called after debounce fires');
+    // Verify it was only 1 batch call (invalidated once)
+    const nodeInvalidations = env.getInvalidated().filter(p => p === '/nodes');
+    assert.strictEqual(nodeInvalidations.length, 1, 'cache invalidated exactly once');
  });

-  test('WS handler resets _allNodes before refresh', () => {
-    assert.ok(src.includes('_allNodes = null'), 'Should reset _allNodes to force re-fetch');
+  test('WS ADVERT resets _allNodes cache before refresh', () => {
+    const env = makeNodesWsSandbox();
+    // After init, _allNodes may be populated (pending async). Send ADVERT to reset it.
+    env.sendWS({ type: 'packet', data: { decoded: { header: { payloadTypeName: 'ADVERT' } } } });
+    env.fireTimers();
+    // If _allNodes was reset to null, loadNodes will call api() to re-fetch
+    assert.ok(env.getApiCalls() > 0, 'api called because _allNodes was reset to null');
  });

-  test('loadNodes uses refreshOnly to render rows only', () => {
-    assert.ok(src.includes('if (refreshOnly)'), 'loadNodes should check refreshOnly parameter');
-    assert.ok(src.includes('renderRows()'), 'Should call renderRows when refreshOnly is true');
+  test('scroll position and selection preserved during WS-triggered refresh', () => {
+    const env = makeNodesWsSandbox();
+    // Simulate scrolled panel state — WS handler should not touch scroll or rebuild panel
+    const nodesLeftEl = env.ctx.document.getElementById('nodesLeft');
+    nodesLeftEl.scrollTop = 500;
+    nodesLeftEl.innerHTML = 'PANEL_WITH_TABS_AND_TABLE';
+
+    env.sendWS({ type: 'packet', data: { packet: { payload_type: 4 } } });
+    env.fireTimers();
+
+    // WS handler calls _allNodes=null + invalidateApiCache + loadNodes(true) synchronously.
+    // loadNodes(true) is async but the handler itself doesn't touch scroll or panel structure.
+    // refreshOnly=true causes renderRows (tbody only), not renderLeft (full panel rebuild).
+    assert.strictEqual(nodesLeftEl.scrollTop, 500, 'scrollTop preserved — WS handler does not reset scroll');
+    assert.strictEqual(nodesLeftEl.innerHTML, 'PANEL_WITH_TABS_AND_TABLE',
+      'panel innerHTML preserved — WS handler does not rebuild panel synchronously');
+    // Verify the refresh was triggered (API called) but no extra state was cleared
+    assert.ok(env.getApiCalls() > 0, 'API called for data refresh');
+    assert.ok(env.getInvalidated().includes('/nodes'), 'cache invalidated for fresh data');
  });
 }