Claude finished @Yeraze's task —— View job

PR Review

• Read changed files
• Review code quality and correctness
• Check test coverage
• Post final feedback

Summary

This PR correctly identifies and fixes a real bug: sendToRadioFrame fires the Sent event without a payload, which meant expectedAckCrc was never populated, tag stayed null, and the tag !== null && tag !== 0 guard in onResp always blocked the BinaryResponse from being accepted. The 15-second inner timeout would fire silently every time. The root cause analysis is accurate.

Code Review (src/server/meshcoreNativeBackend.ts)

The fix logic (lines 956–992):

let sentReceived = false;
let tag: number | null = null;
...
const onSent = (r: any) => {
  sentReceived = true;
  tag = (r?.expectedAckCrc ?? null);
};
const onResp = (r: any) => {
  if (!sentReceived) return;
  if (tag !== null && tag !== 0 && r?.tag !== tag) return;
  cleanup();
  resolve((r?.responseData ?? new Uint8Array()) as Uint8Array);
};

Correctness: The logic is sound. Accepting the first BinaryResponse after Sent when tag is null/0 is reasonable — request_regions calls are sequential (gated by the outer 30s sendCommand timeout and the resolvePublicKey lookup), so a stale BinaryResponse from a prior request is unlikely to race in. The guard if (!sentReceived) return prevents premature acceptance.

Minor observations:

1. tag being null vs 0 conflation (line 976): The condition tag !== null && tag !== 0 treats both null (no tag) and 0 (tag present but zero-valued) as "tag-less mode". A zero expectedAckCrc from firmware is theoretically possible, though in practice CRC=0 is unlikely and the existing tag-matching path (if sendTextMessage-style) would also need it to match. This is an acceptable simplification with a comment explaining it — the PR description covers the rationale.

2. c.on(K.ResponseCodes.Sent, onSent) uses persistent listener (line 989): Using on instead of once means if Sent fires more than once (e.g., firmware quirk), tag gets overwritten. once would be safer here since the first Sent event is the one we care about. The BinaryResponse handler already uses on correctly to allow multiple non-matching responses to pass through. Changing Sent to once would be a small hardening improvement.

3. void c.sendToRadioFrame(frame) (line 992): Consistent with the existing pattern. The return value isn't used, void suppresses the linter warning correctly.

Test Coverage (src/server/meshcoreNativeBackend.discovery.test.ts)

The new test at line 282–316 correctly simulates the bug scenario:

• Overrides sendToRadioFrame to emit Sent with no payload
• Emits BinaryResponse with a firmwareTag value that wouldn't have matched before
• Asserts res.success === true and that the regions are correctly parsed

Good: The test exercises the exact failure mode described in the PR and verifies the fix end-to-end.

One gap: There's no test for the timeout path — i.e., verifying that if neither Sent nor BinaryResponse ever fires, the command rejects after timeoutMs. This isn't a regression introduced by this PR (the timeout existed before), but the new sentReceived guard creates an additional way for the promise to hang (if Sent never fires but BinaryResponse does). Worth noting as a future improvement.

Existing test preserved: The original request_regions sends [57, pubkey, 0x01, 0x00] test (line 255) still passes the expectedAckCrc: 0x99 path, which keeps the tag-matching code exercised. Good.

Verdict

Approve with one minor suggestion: Consider changing c.on(K.ResponseCodes.Sent, onSent) to c.once(...) on line 989 so a duplicate Sent event can't overwrite the tag after BinaryResponse has already been matched. Not a blocker — the fix is correct and well-tested as written.

Claude finished @Yeraze's task —— View job

PR Review

• Read changed files
• Review code quality and correctness
• Check test coverage
• Post final feedback

Summary

This PR correctly identifies and fixes a real bug: sendToRadioFrame fires the Sent event without a payload, so expectedAckCrc was never populated, tag stayed null, and the original guard if (tag === null || r?.tag !== tag) return always blocked the BinaryResponse from being accepted. The 15-second inner timeout would fire silently every time. The root cause analysis is accurate and the fix is well-targeted.

Code Review (src/server/meshcoreNativeBackend.ts)

The fix (lines 955–995):

let sentReceived = false;
let tag: number | null = null;
...
const onSent = (r: any) => {
  sentReceived = true;
  tag = (r?.expectedAckCrc ?? null);
};
const onResp = (r: any) => {
  if (!sentReceived) return;
  if (tag !== null && tag !== 0 && r?.tag !== tag) return;
  cleanup();
  resolve((r?.responseData ?? new Uint8Array()) as Uint8Array);
};
...
c.once(K.ResponseCodes.Sent, onSent);   // ← once, not on
c.on(K.PushCodes.BinaryResponse, onResp);
c.once(K.ResponseCodes.Err, onErr);

Correctness: The logic is sound. The two-variable approach (sentReceived + tag) cleanly separates "did we receive the Sent ack" from "do we have a usable CRC tag". Accepting the first BinaryResponse after Sent when tag is null/0 is correct for the sendToRadioFrame path — request_regions calls are sequential, so a stale BinaryResponse from a prior call is unlikely to race in, and the if (!sentReceived) return guard prevents premature acceptance.

Previous reviewer suggestion already applied: The c.on → c.once for Sent (line 992) has been correctly applied in this revision. This prevents a hypothetical duplicate Sent event from overwriting tag after the response has already been matched.

Minor observations (non-blocking):

1. c.off(K.ResponseCodes.Sent, onSent) in cleanup() (line 983): Since Sent is now registered with once, calling c.off in cleanup() is a no-op if the once already fired and auto-removed the listener. This is harmless — EventEmitter.off is idempotent when the listener isn't registered — but worth knowing it's defensive dead code for the Sent listener only. The BinaryResponse and Err off-calls remain necessary.

2. tag !== null && tag !== 0 condition (line 976): Treats both null (payload absent) and 0 (CRC explicitly zero) as "tag-less mode". CRC=0 from firmware is theoretically possible but extremely unlikely. The comment in the diff explains the rationale. Acceptable.

3. onErr uses once (line 994): Correct — only one Err response expected. Consistent with the Sent listener.

Test Coverage (src/server/meshcoreNativeBackend.discovery.test.ts)

New test (lines 282–316) correctly simulates the exact bug scenario:

• Overrides sendToRadioFrame to emit Sent without any payload
• Emits BinaryResponse with tag: 0xdeadbeef (a non-zero firmware tag that wouldn't have matched before)
• Asserts res.success === true and correct region/clock parsing

This is a good regression test — it would have caught the original bug.

Existing test preserved (line 255): The original request_regions sends [57, pubkey, 0x01, 0x00] test still exercises the tag-matching code path (the mock emits expectedAckCrc: 0x99). Both paths are now covered.

One potential gap (future improvement, not a blocker): No test for the case where Sent never fires but BinaryResponse arrives — with the new sentReceived guard this would silently hang until the outer timeout. This is a pre-existing limitation and not introduced by this PR, but worth a follow-up test.

Verdict

Approve. The fix is correct, the c.once suggestion from the prior review has been incorporated, the comments explain the rationale clearly, and the regression test covers the failure mode directly. No blocking issues.