fix(meshtastic): only push add_contact when the radio lacks the peer's key (#4368)

[Yeraze]

Fixes #4368.

Problem

MeshMonitor sent an add_contact admin message before every PKI-encrypted DM, with no check for whether the radio already had that contact (meshtasticManager.ts, the pushContactToRadio call in sendTextMessage).

That is not a free no-op. The firmware's NodeDB::addFromContact() sets is_favorite = true to shield the entry from NodeDB eviction:

"Boring old nodes" are the first to be evicted out of the node database when full. This includes a newly-zeroed nodeinfo because it has: !is_favorite && last_heard==0. To keep this from happening when we addFromContact, we set the new node as a favorite.

So every DM — including every automated Auto-Ack reply — re-favorited its recipient. The favorite then came back through the NodeInfo downsync, which made it look like foreign device state was leaking in when MeshMonitor had caused it.

This matches the reported symptom exactly: every DM recipient favorited, regardless of hop count or role. The zero-cost-hop auto-favorite feature (0-hop + relay-role gated) could never explain that.

Fix

New deviceContactKeyNums set: nodes we have positive evidence the radio already holds a public key for.

Deliberately distinct from deviceNodeNums, which only means the radio knows the node exists — it can know a node without holding its key, and guarding on that alone would silently break PKI DMs in exactly that case.

Evidence recorded when:

• the device's own NodeDB dump reports user.publicKey — most authoritative, the radio stating what it holds
• a mesh NodeInfo carrying a key reaches us through the radio, so the radio saw and stored the same key
• we successfully push a contact ourselves

Evidence invalidated when:

• the connection resets — a different radio, or one whose NodeDB was wiped while we were away, may not hold the same keys
• a key repair purges the node via removeDeviceNodeNum — the purge takes the key with it, so the next DM must push again

Net effect: the radio still gets the key whenever it genuinely lacks one, so PKI DMs keep working — but a node is no longer re-favorited on every message.

What this deliberately does NOT do

• No filtering of the isFavorite downsync. An earlier revision of [BUG] add_contact is re-sent before every PKI DM, so the firmware re-favorites every DM recipient #4368 proposed that. It would have left MeshMonitor favoriting nodes on the device while hiding the fact from the UI, and regressed [BUG] Favorites are not updated after reconnect to node #213 (offline favorite changes). The downsync is correct and untouched.
• No CLIENT_BASE special-casing. Current firmware already implements the meshtastic/design#39 exception, in both addFromContact and AdminModule.cpp:110 (admin-key auto-favorite). That alignment belongs at the firmware layer, and duplicating it here would be redundant.

Testing

9 new tests in meshtasticManager.contactPushGuard.test.ts, driving the real sendTextMessage and the real NodeInfo-processing methods rather than re-implementing the predicate (the pattern the existing pushContactToRadio guard block in meshtasticManager.test.ts uses).

Verified non-vacuous — with the guard reverted, 3 of the 9 fail:

• does NOT re-push on subsequent DMs to the same node
• never pushes when the radio already reported the key from its own NodeDB
• never pushes when a mesh NodeInfo carrying the key reached us through the radio

Also covered: first-DM still pushes, re-push after key-repair purge, per-connection reset, per-node isolation, and the two no-push paths (key mismatch, no public key).

tsc --noEmit clean; lint:ci ratchet clean.

Full local suite: 11,112 passed, 0 failed (679 skipped = the PostgreSQL/MySQL suites; no schema or migration work in this PR).

🤖 Generated with Claude Code

https://claude.ai/code/session_01L9NzRtqE8eSMS8tvAeodUB

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(meshtastic): only push add_contact when the radio lacks the peer's key

Tasks:

• Gather context and understand the request
• Read changed files
• Review implementation in meshtasticManager.ts
• Review test coverage in contactPushGuard.test.ts
• Provide comprehensive feedback

---

Overall Assessment

This is a well-reasoned, carefully scoped fix with good documentation and solid test coverage. The approach is correct: tracking key evidence separately from node presence is the right design. Below are observations across quality, correctness, and coverage.

---

Implementation (meshtasticManager.ts)

Design correctness — ✅ Solid

The decision to use a separate deviceContactKeyNums set distinct from deviceNodeNums is explicitly justified (lines 850–860) and avoids the failure mode where a node is known but key-less. This is the right design.

Evidence is recorded in all three places described in the PR:

• processNodeInfoProtobuf (line 7056) — mesh NodeInfo routing through the radio
• processNodeInfoMessageProtobuf-equivalent path (line 8671) — device's own NodeDB dump
• pushContactToRadio after a successful push (line 12950)

And invalidated in the right places:

• Connection reset (line 1679)
• removeDeviceNodeNum (line 13411)

Potential issue — comment in error handler (lines 9073–9078)

} catch {
  // Non-fatal — radio may already have the contact, or the send failed
  // transiently. On failure deviceNodeNums is left untouched (the add only
  // runs after a successful send), so the UI's "not in device DB" warning
  // correctly lingers until a later push or NodeInfo confirms the contact.
}

The comment says "On failure deviceNodeNums is left untouched" — but deviceContactKeyNums is also left untouched on failure (because pushContactToRadio updates deviceContactKeyNums at line 12950, after the transport.send at line 12943). That part is actually correct behavior — the comment is just incomplete. Minor: the comment should mention deviceContactKeyNums too since it's the new set readers will notice.

Observation — pushContactToRadio early-return silently skips deviceContactKeyNums update (lines 12926–12931)

private async pushContactToRadio(...) {
  if (!this.isConnected || !this.transport || !this.localNodeInfo?.nodeNum) {
    return;  // ← silent skip
  }
  if (!targetNode.publicKey || !targetNode.nodeId || ...) {
    return;  // ← silent skip
  }
  ...
  this.deviceContactKeyNums.add(targetNode.nodeNum);
}

When pushContactToRadio returns early (no connection, or incomplete node data), deviceContactKeyNums is NOT updated. This is intentional and correct — we don't want to mark the key as present if we couldn't push. But the caller catches all exceptions, so if a push silently no-ops (e.g., missing localNodeInfo), the guard deviceContactKeyNums.has(destination) remains false and the next DM will try again. This is the safe failure mode. No bug, but the early-return paths could benefit from a debug log similar to the existing one at line 12951 so failures are traceable.

Observation — processNodeInfoMessageProtobuf (device NodeDB dump, line 8671)

The comment reads "The radio is reporting a key out of its OWN NodeDB." This is accurate for the device-sync path where the device dumps its NodeDB. But this same code path also handles mesh NodeInfo messages from remote nodes, as the comment at line 8672 clarifies. The distinction here is whether this is processNodeInfoProtobuf (mesh) vs the device-sync processing. Looking at both sites, the reasoning is sound — both are valid evidence the radio holds the key.

Minor: Number(nodeInfo.num) coercion (line 8671)

Consistent with the multi-database BIGINT handling guidelines in CLAUDE.md. ✅

---

Test Coverage (meshtasticManager.contactPushGuard.test.ts)

Coverage — ✅ Comprehensive

The 9 test cases cover:

1. First DM pushes (positive case)
2. Subsequent DMs do NOT push (regression case — the actual bug)
3. No push when device NodeDB dump shows the key
4. No push when mesh NodeInfo routing shows the key
5. Re-push after key-repair purge
6. Per-connection reset (clearing deviceContactKeyNums simulates reconnect)
7. Key mismatch — no push (PKI skipped entirely)
8. No public key — no push
9. Per-node isolation (knowing one peer says nothing about another)

The PR author notes 3 of 9 fail without the fix — this is a good non-vacuousness check.

Test pattern — a mild concern

The test mocks pushContactToRadio at line 66:

pushSpy = vi.spyOn(manager as any, 'pushContactToRadio').mockImplementation(async (n: any) => {
  (manager as any).deviceNodeNums.add(n.nodeNum);
  (manager as any).deviceContactKeyNums.add(n.nodeNum);
});

The mock manually replicates pushContactToRadio's side effects (adding to both sets). This means the tests verify the guard (deviceContactKeyNums.has) but don't exercise the real push path or verify that the real pushContactToRadio correctly adds to deviceContactKeyNums after a successful send. That said, this is a deliberate trade-off (noted in the PR body) to avoid needing a live transport. The existing integration tests cover the push path itself.

Missing edge case: what happens if pushContactToRadio throws?

The guard in sendTextMessage swallows all exceptions (line 9073). If pushContactToRadio throws, deviceContactKeyNums is never updated, so the next call retries. This is correct behavior but isn't explicitly tested. A test that makes pushSpy throw and then verifies that the next DM retries the push would give better regression protection for that error path.

---

Security Considerations

No new attack surface introduced. The guard operates entirely on server-internal state and doesn't change what gets encrypted or how.

---

Performance Considerations

The Set.has() check is O(1) and negligible. This is strictly a performance improvement — removing a network round-trip admin message before every PKI DM.

---

Summary

This is a clean, well-motivated fix. The implementation correctly models key evidence as distinct from node presence, invalidates it in all the right places, and the tests are genuinely regression-useful (3 fail without the fix).

Suggested improvements (non-blocking):

1. Update the error handler comment at line 9074 to also mention deviceContactKeyNums is untouched on failure.
2. Add debug logging to pushContactToRadio early-return paths for traceability.
3. (Optional) Add a test for the push-failure → retry-on-next-DM path.