Claude finished @Yeraze's task —— View job

PR Review: fix(mqtt): attribute same-key channels by packet hash, not sort order

Tasks:

• Gather context and understand the PR
• Review channelDecryptionService.ts
• Review channelDecryptionService.test.ts
• Review mqttIngestion.ts
• Review docs/features/channel-database.md
• Post comprehensive feedback

Summary

This is a well-designed fix for a real attribution bug. The core logic is sound, tests are thorough, and the fallback-not-hard-skip design avoids the regression that enforcing a hard channel hash match would cause on non-LongFast modem presets. The Meshtastic channel hash algorithm (xorHash(name) ^ xorHash(psk) using the expanded key) is implemented correctly.

Issues Found

🔴 Bug: import statements after function body in mqttIngestion.ts

src/server/mqttIngestion.ts:75-94 — The new bootstrapMqttChannelDatabase function (lines 40–74) is placed before the import statements it depends on. The logger import is at line 94, but logger.info is called at line 64. This is technically legal in ES modules (imports are hoisted), but it is a clear code organization error — especially notable since the function uses logger before its import appears in the file. The imports should be at the top of the file.

// Lines 64-66 use logger, but logger isn't imported until line 94:
logger.info(
  `MQTT source ${sourceId} bootstrapped channel_database entry "${ch.name}" with default key`,
);
// ...
// Line 94:
import { logger } from '../utils/logger.js';

Action required: Move the bootstrapMqttChannelDatabase function and the new top-of-file imports so that all imports appear first, or move the function below the existing imports.

🟡 Issue: Hash collision false-preference with same-hash, same-key channels

In tryDecrypt() (line 419), the filter candidates.filter((c) => c.expectedChannelHash === channelHash) groups all channels with a matching hash into the "prefer" group — including channels that share the same key. If two same-key channels have the same hash (highly unlikely but theoretically possible: their names XOR to the same bytes), both end up in the preferred group and the first by sortOrder wins, which is the pre-fix behavior for that sub-case. This edge case is already acknowledged by the PR's "1-byte hash collision" fallback test, so it's an accepted limitation, but worth noting.

🟡 Issue: Missing test for sortOrder tie-breaking within the same-hash group

The new test block (Hash-aware attribution (same-key channels)) confirms hash-matching channels are tried first, but there's no test verifying that within the expectedChannelHash === channelHash group, sortOrder is still the tiebreaker. The pre-sort at cache-refresh time (sortedChannels) ensures this, but an explicit test would prevent a regression if the ordering logic is ever changed.

🟡 Minor: encryptTestData helper is duplicated three times

encryptTestData (the nonce-building + AES-CTR encrypt helper) is defined in three separate describe blocks (Decryption, Decryption with Specific Channel, Channel Hash Validation, and a fourth cleaner signature in Hash-aware attribution). Extracting it to a single file-level helper would reduce 30+ lines of duplication.

🟢 Observation: computeChannelHash export is good for testability

Exporting xorHash and computeChannelHash at the bottom of the service file (line 510) is the right call — the new test block uses them directly. This is clean.

🟢 Observation: Hash algorithm matches firmware

The xorHash(name) ^ xorHash(expandedPsk) algorithm is confirmed correct against the Meshtastic firmware's Channels.cpp — and the test at line 621 correctly uses expandShorthandPsk before computing the reference hash, matching how the service caches it. This is the most important correctness check and it passes.

🟢 Observation: enforceNameValidation remains orthogonal

The hard-skip logic at lines 403–411 is unchanged in semantics — it filters candidates before the preference-ordering step. The new test hard-skip still wins over preference (line 735) correctly validates that adding hash-preference didn't weaken the existing hard-skip guarantee. Well done.

Minor Documentation Nit

docs/features/channel-database.md:143 — "How Server-Side Decryption Works" step 3 still says "in sort order" without mentioning hash-preferred order. Step 3 now reads: "If your node cannot decrypt it, MeshMonitor tries each enabled Channel Database entry in sort order". This should be updated to reflect the new hash-preferred behavior, e.g., "hash-matched channels first, then remaining channels in sort order." The other sections (Channel Priority and Ordering tip, Enforce Name Validation) are already updated, so this is the only stale reference.

Verdict

The fix is correct and production-ready. The critical bug (imports-after-function-body in mqttIngestion.ts) should be addressed before merge. The other findings are non-blocking.