Claude finished @Yeraze's task —— View job

PR Review

• Gather context and read changed files
• Review code quality and correctness
• Check test coverage
• Post feedback

Summary

This is a well-targeted, minimal fix for a timing-dependent bug. The root cause analysis in the PR description is excellent and the fix is correct.

Code Review

src/server/meshcoreVirtualNodeServer.ts — handleDeviceQuery() (line 355–374)

The fix is correct. The firmwareVer field in DeviceInfo is the companion protocol negotiation field, not a display value. Pinning it to SUPPORTED_COMPANION_PROTOCOL_VERSION (always 1) is the right behavior for the virtual node, which only implements v1 wire frames.

One minor style note: The localNode variable is fetched but only used for display fields (firmwareBuild, model). If localNode is null (manager not yet connected), the function still sends a valid DeviceInfo response using defaults — this is arguably the correct behavior for DeviceQuery (unlike AppStart which correctly early-exits with BadState). The current behavior is fine, but a comment acknowledging the intentional difference from handleAppStart would add clarity to future readers.

// Note: intentionally does NOT gate on isConnected() — DeviceQuery is sent
// by the app BEFORE AppStart, so we must reply even with a cold manager.
// Display fields fall back to safe defaults; the version byte is always pinned.

This is purely a suggestion, not a required change.

No issues with the fix itself.

src/server/meshcoreVirtualNodeServer.test.ts

Test coverage is strong. Two improvements were made:

1. Line 179–184 — Strengthened the existing DeviceQuery test to assert payload.readInt8(1) === SUPPORTED_COMPANION_PROTOCOL_VERSION. Previously it only checked the response code; now it validates the protocol version byte, which is the precise field the bug was in.

2. Lines 186–207 — New regression test constructs a manager with firmwareVer: 7 (simulating the warm-cache state from the issue) and asserts:

   • payload[0] is ResponseCodes.DeviceInfo
   • payload.readInt8(1) equals SUPPORTED_COMPANION_PROTOCOL_VERSION
   • payload.readInt8(1) is not 7

   The dual assertion (both positive and negative) makes the regression intent maximally clear. Good pattern.

Minor: The regression test spins up a second full server instance with a try/finally. This works correctly. An alternative would be to expose a test helper or reuse the beforeEach manager via mutation (e.g., (manager as any).localNode.firmwareVer = 7), but the current approach avoids coupling to internal implementation details of FakeManager, which is the right call.

Verdict

✅ Approve — The fix is correct, minimal, and well-tested. The regression test directly covers the bug scenario described in the issue. No functional concerns.

Thanks for the review! Applied the optional clarity suggestion in the latest commit — added a comment on handleDeviceQuery noting it intentionally does not gate on isConnected()/localNode (unlike handleAppStart), because the app sends DeviceQuery before AppStart and we must reply even with a cold manager. No logic change.

Claude finished @Yeraze's task —— View job

PR Review

• Gather context and read changed files
• Review code quality and correctness
• Check test coverage
• Post feedback

Summary

This follow-up commit adds the clarity comment suggested in the previous review. The change is purely documentation — no logic was altered.

handleDeviceQuery() comment (lines 357–372)

The comment is accurate and informative. A few observations:

Positive: It correctly explains both why localNode gating is intentionally absent (unlike handleAppStart) and why the version byte must always be SUPPORTED_COMPANION_PROTOCOL_VERSION. The comment also cross-references the issue (#3705) and describes the exact failure mode, which is exactly the right level of detail for a subtle timing-dependent behavior.

Minor nit (non-blocking): The comment is quite long (11 lines). The last paragraph is a full re-statement of the root cause from the PR description. For source-level comments, a shorter form is often preferred — something like:

// Does NOT gate on isConnected()/localNode (unlike handleAppStart, which
// replies BadState): DeviceQuery arrives *before* AppStart, so we must
// reply even with a cold manager; display fields fall back to safe defaults.
//
// IMPORTANT: firmwareVer must always be SUPPORTED_COMPANION_PROTOCOL_VERSION —
// NOT the real node's firmwareVer. This byte is the companion *protocol*
// negotiation field. Leaking the real fw version causes the app to abort the
// handshake (see issue #3705).

This is purely a style preference — the current version is not wrong, just verbose. Given that the PR description already documents the full root cause, the in-code comment doesn't need to repeat all of it.

Verdict

✅ Approve — The clarifying comment fulfills the review suggestion and accurately documents the intentional design difference. No functional concerns. The fix, tests, and documentation are all correct.