Claude finished @Yeraze's task —— View job

PR Review — fix(meshcore): correlate buffered SNR/route to the matching packet (#3589)

• Gathering context
• Reading changed files
• Reviewing code quality, bugs, performance, security
• Reviewing test coverage
• Posting final review

Summary

This is a well-reasoned fix for a real protocol-level problem. The root cause analysis is accurate, the implementation is clean and testable, and the new tests directly exercise the failure modes that caused the original bug. Here's the detailed review:

Code Quality & Design

consumePendingPath helper (meshcoreNativeBackend.ts:371-390) — the extraction of a reusable consumePendingPath(msgPathLen) method is excellent. It cleanly enforces consume-once semantics in a single place, which is exactly the right abstraction here. The dual-guard design (freshness + pathLen correlation) is sound.

bufferedAt timestamp — storing Date.now() at buffer-write time and checking it at consume time is a clean way to detect stale buffers. The 500ms window (PENDING_PATH_MAX_AGE_MS) is well-justified in the comment and appropriate for the firmware's same-I/O-tick delivery guarantee.

Room post fix (meshcoreNativeBackend.ts:475) — this was a real leak: isRoomPost branch returned early without consuming the buffer, allowing the stale path/SNR to bleed onto the next unrelated message. The fix (calling consumePendingPath(msg.pathLen) before the early return) is correct.

Potential Issue: pathLen correlation edge case

consumePendingPath when msgPathLen is not a number (meshcoreNativeBackend.ts:381-388):

if (
  typeof msgPathLen === 'number' &&
  typeof buffered.rawPathLen === 'number' &&
  msgPathLen !== buffered.rawPathLen
) {
  // Buffer is from a different packet (packed pathLen bytes differ).
  return undefined;
}

When msgPathLen is undefined or not a number, the pathLen correlation check is skipped entirely, and a fresh buffer is always attached. This is explicitly intended as a fallback for firmware that doesn't provide pathLen. However, this fallback means a stale-but-fresh (within 500ms) buffer from a different packet will still be attached if the recv event has no pathLen. This is an acceptable trade-off given the freshness window, but worth noting since it's a subtle assumption.

Minor observations

1. snr: undefined explicitly set on room posts (meshcoreNativeBackend.ts:487) — this is consistent with the intent (not surfacing SNR on room posts), but the field could simply be omitted since it's the same as not including it. Minor style point, no functional issue.

2. bufferedAt field in the private type — the inline type definition of pendingTxtMsgPath (meshcoreNativeBackend.ts:220) includes bufferedAt: number which is set on write and checked on consume. Clean; no issues.

3. The PENDING_PATH_MAX_AGE_MS is declared as a static readonly (meshcoreNativeBackend.ts:234). This is a good choice — it means tests can verify the constant's value directly if needed, even though current tests use vi.useFakeTimers() to advance time past it (which is the right approach).

Test Coverage

The 4 new test cases in meshcoreNativeBackend.otaPacket.test.ts directly and specifically test the failure modes:

The fake timer test correctly uses try/finally to restore real timers — good defensive testing practice.

Gap to consider: There's no test for a ChannelMsgRecv with a mismatched pathLen. The consumePendingPath helper is shared, so the contact_message tests implicitly cover it, but a symmetrical channel_message pathLen-mismatch test would make the coverage more explicit.

Security & Performance

No security concerns. No performance concerns — the single-slot buffer and lightweight timestamp check are negligible overhead.

Verdict

Approve. The fix is correct, minimal, well-documented, and the new tests are targeted at the exact failure modes. The consumePendingPath abstraction is clean and leaves the code in better shape than before. The edge case around msgPathLen not being a number is a known intentional trade-off.