fix: PKI_UNKNOWN_PUBKEY errors now correctly flag key mismatch

[Yeraze]

Summary

PKI_UNKNOWN_PUBKEY and PKI_SEND_FAIL_PUBLIC_KEY routing errors were not triggering key mismatch detection when the target node wasn't in the radio's device database.

Root Cause

Both routing error paths (request packets at line ~5349, message-tracked packets at line ~5406) had a guard:

if ((errorReason === PKI_UNKNOWN_PUBKEY || errorReason === PKI_SEND_FAIL_PUBLIC_KEY) 
    && !this.isNodeInDeviceDb(toNum)) {
  // Silently swallowed — no keyMismatchDetected flag set
}

This assumed the error was only expected after factory reset/purge, but it also suppressed detection for legitimate cases where the radio doesn't know the node's public key.

Fix

Removed the isNodeInDeviceDb() suppression guard. All PKI errors now flag keyMismatchDetected = true regardless of device DB state. The flag clears naturally when keys are re-synced (via PR #2365's resolution logic).

Files Changed

File	Change
src/server/meshtasticManager.ts	Remove isNodeInDeviceDb guard from both PKI error paths

Test plan

• 3061 tests pass, 0 failures

🤖 Generated with Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: PKI_UNKNOWN_PUBKEY Error Detection Fix

Reviewing this pull request for code quality, potential bugs, performance, security concerns, and test coverage.

Review Checklist:

• Read and understand the code changes
• Analyze the root cause and fix approach
• Check for potential bugs or edge cases
• Evaluate security implications
• Assess performance impact
• Review test coverage adequacy
• Provide constructive feedback

---

Code Review Summary

✅ Positive Aspects

1. Clear Problem Identification: The root cause analysis is excellent - the isNodeInDeviceDb() guard was indeed suppressing legitimate PKI errors that should trigger key mismatch detection.

2. Consistent Fix: The change is applied uniformly to both error handling paths:

   • Request packets (line ~5349)
   • Message-tracked packets (line ~5406)

3. Code Simplification: Removing the conditional logic reduces complexity and eliminates a potential source of bugs.

4. Proper Logging: The fix maintains appropriate logging levels (changed from logger.info to logger.warn for actual errors).

⚠️ Areas for Consideration

1. Test Coverage Gap: While 3061 tests pass, I don't see specific tests for PKI error handling scenarios. Consider adding tests that verify:

   // Test case suggestions:
   - PKI_UNKNOWN_PUBKEY with node not in device DB → should flag keyMismatchDetected
   - PKI_SEND_FAIL_PUBLIC_KEY with node not in device DB → should flag keyMismatchDetected  
   - PKI_FAILED scenarios → should always flag keyMismatchDetected

2. Edge Case Documentation: The removed code had a specific assumption about post-factory-reset scenarios. Consider adding a comment explaining why this assumption was incorrect:

   // PKI errors should always trigger key mismatch detection regardless of device DB state
   // as the radio not knowing a node's key can occur in legitimate scenarios beyond factory reset

3. Potential Noise: This change may increase the frequency of keyMismatchDetected flags. While this is the correct behavior, consider monitoring for:

   • False positive rates in production
   • User confusion from more frequent key mismatch notifications

🔒 Security Assessment

Positive security impact: This fix improves security posture by ensuring legitimate PKI errors aren't silently ignored. Key mismatch detection is a critical security feature that helps identify:

• Stale keys after device changes
• Potential man-in-the-middle attacks
• Configuration drift between devices

🚀 Performance Impact

Minimal performance impact: The change removes conditional checks, actually slightly improving performance. Database operations remain the same.

📝 Code Quality

Good:

• Follows established error handling patterns
• Maintains consistent logging and event emission
• Aligns with the repository's async database service patterns (meshtasticManager.ts:5353-5358)

Final Recommendation

✅ APPROVE - This is a well-reasoned fix that addresses a real bug in PKI error detection. The change is minimal, safe, and improves the system's security posture.

Suggested follow-ups (not blockers):

1. Add specific unit tests for PKI error scenarios
2. Monitor production logs for any unexpected increase in key mismatch detections
3. Consider adding telemetry to track PKI error patterns

---

Job Run | Branch: fix/pki-unknown-pubkey-detection

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: PKI_UNKNOWN_PUBKEY Error Detection Fix

Reviewing this pull request for code quality, potential bugs, performance, security concerns, and test coverage.

Review Checklist:

• Read and understand the code changes
• Analyze the root cause and fix approach
• Check for potential bugs or edge cases
• Evaluate security implications
• Assess performance impact
• Review test coverage adequacy
• Provide constructive feedback

---

Code Review Summary

✅ Positive Aspects

1. Excellent Problem Identification: The root cause analysis is spot-on. The isNodeInDeviceDb() guard was indeed suppressing legitimate PKI errors that should trigger key mismatch detection. The fix aligns perfectly with the documented principle in ARCHITECTURE_LESSONS.md:846: "Never suppress PKI error detection based on device DB state."

2. Consistent Implementation: The change is applied uniformly to both error handling paths:

   • Request packets (src/server/meshtasticManager.ts:5346)
   • Message-tracked packets (src/server/meshtasticManager.ts:5392)

3. Code Simplification: Removing conditional logic reduces complexity and eliminates potential bugs while making the behavior more predictable.

4. Comprehensive Test Coverage: Added 59 new test lines covering:

   • All PKI error constants validation
   • isPkiError() helper function behavior
   • Explicit documentation of the behavioral change in test comments
   • getPortNumName() coverage for completeness

5. Documentation Excellence: Updated ARCHITECTURE_LESSONS.md with:

   • Clear problem statement and solution
   • Anti-pattern guidance
   • Specific file locations for reference
   • Added to related PRs list for historical tracking

🔒 Security Assessment

Significant security improvement: This fix enhances the security posture by ensuring PKI errors aren't silently ignored. Key mismatch detection is critical for identifying:

• Stale keys after device resets/changes
• Potential man-in-the-middle scenarios
• Configuration drift between mesh devices

The fix correctly prioritizes security over convenience - it's better to flag potential key issues and let them resolve naturally than to miss legitimate security problems.

🚀 Performance Impact

Positive performance impact: Removing conditional checks and device DB lookups actually improves performance slightly. The change reduces code paths and database queries.

📝 Code Quality

Excellent quality:

• Maintains existing error handling patterns
• Preserves appropriate logging levels
• Uses shared constants (isPkiError()) consistently
• Follows the repository's async database service conventions

🧪 Test Coverage

Comprehensive coverage:

• New tests document the exact behavioral change
• Tests verify all three PKI error codes (PKI_FAILED, PKI_UNKNOWN_PUBKEY, PKI_SEND_FAIL_PUBLIC_KEY)
• Validates the isPkiError() helper function behavior
• All 3061 existing tests continue to pass

⚠️ Minor Considerations

1. Monitoring Recommendation: This change may increase the frequency of keyMismatchDetected flags in production. This is the correct behavior, but monitoring logs for patterns could be valuable.

2. User Experience: More frequent key mismatch notifications might initially confuse users, but this accurately reflects the mesh state and improves long-term reliability.

Final Recommendation

✅ STRONGLY APPROVE - This is an exemplary bug fix that:

• Addresses a real security issue
• Includes comprehensive testing
• Provides excellent documentation
• Follows established architectural patterns
• Has minimal risk and positive impact

The change correctly implements the architectural principle that "All PKI errors must trigger key mismatch detection regardless of device database state." The implementation is clean, well-tested, and properly documented.

---

Job Run | Branch: fix/pki-unknown-pubkey-detection