Skip to content

feat(meshcore-vn): relay SendStatusReq(27) status requests to the physical node (#3904)#3991

Merged
Yeraze merged 1 commit into
mainfrom
fix/3904-vn-status-neighbours
Jul 7, 2026
Merged

feat(meshcore-vn): relay SendStatusReq(27) status requests to the physical node (#3904)#3991
Yeraze merged 1 commit into
mainfrom
fix/3904-vn-status-neighbours

Conversation

@Yeraze

@Yeraze Yeraze commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes the status / owner-info action failing after a successful remote login through the MeshCore Virtual Node (issue #3904, second independent report #3975). After login the meshcore-flutter app sends SendStatusReq(27), which fell through to default: in MeshCoreVirtualNodeServer.dispatchCommand() and got an unconditional Err(UnsupportedCmd) — so the action failed as if the login had only granted guest access, even though the login itself succeeded.

This continues the transaction-relay pattern established in #3959 / #3961 (SendSelfAdvert / SendLogin / SendTracePath / SendTelemetryReq): reply Sent, run the operation against the physical node, then push the correlated result back the way meshcore-flutter matches it.

What changed

SendStatusReq(27) — implemented and relayed:

  • Parse [27][pubkey:32] (no reserved bytes, unlike SendTelemetryReq).
  • Reply Sent, call manager.requestNodeStatus(pubkey), then push StatusResponse(0x87) = [0x87][reserved:1][pubKeyPrefix:6][statusData], correlated by the remote's 6-byte pubkey prefix (same correlation the app uses for login/telemetry).
  • The parsed MeshCoreStatus is re-serialized into the 48-byte RepeaterStats wire blob the app's decoder reads (meshcore.js getStatus).
  • Ungated (not tied to allowAdminCommands) — it's a read-only follow-up to login, exactly like SendTelemetryReq / SendTracePath. The real node answers a status request based on the established session, not on MeshMonitor's admin flag.
  • No-result / timeout / manager-throws: emit nothing and let the app time out — same graceful behavior as the existing login/telemetry handlers.

New codec helpers: parseSendStatusReq, encodeStatusResponsePush, encodeRepeaterStatusData, and PushCodes.StatusResponse = 0x87.

Why SendRawData(25) / neighbours is NOT in this PR

The issue assumed SendRawData(25) carries the neighbours request. Firmware inspection (ripplebiz/MeshCore v1.16.0, bbb58cc) proves that assumption is wrong, so shipping a cmd-25 "neighbours" handler would be a guessed, incorrect protocol frame:

  • CMD_SEND_RAW_DATA(25) is a raw-transmit primitive, not a request/response. Layout [25][path_len:int8][path][raw_frame>=4]; the firmware builds a PAYLOAD_TYPE_RAW_CUSTOM packet, transmits it over the air, and replies Ok. Responses (if any) arrive asynchronously and uncorrelated via RawData(0x84) and/or LogRxData(0x88) — no tag, no pubkey prefix. A repeater has no RAW_CUSTOM handler and silently ignores it, so cmd 25 can never produce a neighbours list.
  • Neighbours actually flows via CMD_SEND_BINARY_REQ(50) (REQ_TYPE_GET_NEIGHBOURS 0x06) → BinaryResponse(0x8C), tag-correlated (the tag originates on the responding node).

Two things block a correct neighbours fix in this environment:

  1. Can't confirm the app's actual frame. meshcore-flutter is closed-source; the maintainer observed cmd 25 (not cmd 50) arriving post-login, which contradicts the firmware's only real neighbours path (cmd 50). Determining what the app expects requires a live app + hardware capture.
  2. Manager gaps. There is no raw-transmit method to faithfully forward cmd 25 (replying a fake Ok without actually transmitting would mislead the app), and manager.getNeighbours does not accept the app's requested pubkey_prefix_len, risking a per-neighbour wire-stride mismatch on the 0x8C push.

Rather than ship a wrong frame that would silently fail on real hardware, neighbours is left as a follow-up (recommend implementing it on SendBinaryReq(50) with a hardware capture to confirm what the flutter app sends). This PR therefore references #3904 rather than closing it — status is fixed; neighbours remains.

Testing

  • New VN tests mirroring the SendTelemetryReq suite: happy path (Sent ack + StatusResponse push + prefix correlation + full 48-byte RepeaterStats layout assertions), ungated behavior, no-result, manager-throws, and bad-payload → Err(IllegalArg).
  • Full Vitest suite: 8197 passed, 0 failed, 0 suites failed (--reporter=json success:true).
  • tsc --noEmit: clean (0 errors).
  • ESLint: no new findings (the 4 pre-existing no-useless-assignment errors in meshcoreCompanionCodec.ts are unchanged baseline).

⚠️ Not hardware-validated

Unlike the prior commands in this issue (#3959 / #3961), this change was NOT validated against a live MeshCore node in this environment. @djvdberg94-hash / @Yeraze — please pull this and re-test the status / owner-info action from the app against a real logged-in repeater to confirm the stats come back correctly. (The neighbours action will still fail until the follow-up above lands.)

🤖 Generated with Claude Code

https://claude.ai/code/session_015AFBA76hsjqhsXe1BdnYub

…l node (#3904)

After a successful remote login through the MeshCore Virtual Node, the
meshcore-flutter app's "status"/owner-info action sent SendStatusReq(27),
which fell through to `default:` in `dispatchCommand` and returned
`Err(UnsupportedCmd)` — so the action failed as if login had granted only
guest access (issue #3904, second report #3975).

Mirror the #3961 transaction pattern (Sent ack → async relay → correlated
push):
- Parse SendStatusReq `[27][pubkey:32]` (no reserved bytes, unlike
  SendTelemetryReq), verified against ripplebiz/MeshCore firmware.
- Reply `Sent`, call `manager.requestNodeStatus(pubkey)`, then push
  StatusResponse(0x87) `[0x87][reserved][pubKeyPrefix:6][statusData]`,
  correlated by the remote's 6-byte pubkey prefix.
- Re-encode the parsed `MeshCoreStatus` into the 48-byte `RepeaterStats`
  wire blob the app's decoder reads (meshcore.js `getStatus`). Firmware
  >=1.16 appends two counters (56 bytes) the companion status view does not
  parse, so the 48-byte prefix is exactly what the app renders.
- Ungated (read-only follow-up to login, like SendTelemetryReq/SendTracePath).
- On no-result / failure, emit nothing and let the app time out (matches
  real-node behaviour and the existing login/telemetry handlers).

SendRawData(25) is intentionally NOT implemented here: firmware inspection
proved it is a raw-transmit primitive (reply Ok, async uncorrelated
0x84/0x88 pushes), NOT the neighbours request the issue assumed. Neighbours
actually flows via SendBinaryReq(50) -> BinaryResponse(0x8C). See PR body.

Adds codec helpers `parseSendStatusReq`, `encodeStatusResponsePush`,
`encodeRepeaterStatusData`, `PushCodes.StatusResponse`, and VN tests
mirroring the SendTelemetryReq suite (happy path + correlation, no-result,
manager-throws, bad-payload -> IllegalArg, ungated behaviour).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFBA76hsjqhsXe1BdnYub
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @Yeraze's task —— View job


PR Review: feat(meshcore-vn): relay SendStatusReq(27) status requests to the physical node

Tasks:

  • Gather context and understand the request
  • Read changed files
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Review test coverage
  • Post final review

Overall this is well-structured work that follows the established relay pattern cleanly. Two issues found — one is a real bug that will produce a wrong last_snr value at the app, the other is a misleading test.

🔴 Bug: lastSnr encoding is wrong in encodeRepeaterStatusData

File: src/server/meshcoreCompanionCodec.ts:741

MeshCoreStatus.lastSnr arrives in dB (the native backend already divides by 4 at meshcoreNativeBackend.ts:1477). But encodeRepeaterStatusData writes it raw:

b.writeInt16LE(clampInt16(s.lastSnr ?? 0), o); // last_snr

The wire layout the app expects (meshcore.js getStatus) is a raw quarter-dB int16, which the app then divides by 4 to display. So the fix is to multiply back:

b.writeInt16LE(clampInt16(Math.round((s.lastSnr ?? 0) * 4)), o); // last_snr (quarter-dB)

Without this, a lastSnr of 6.0 dB encodes as wire 6 → app reads 6/4 = 1.5 dB (4× too small).

The test at line 89 also reflects this bug: the mock is annotated // int16 quarter-dB raw value as the wire carries it and uses lastSnr: 24, but in the real pipeline requestNodeStatus() returns a dB value like 6.0 (24/4). The test passes only because both the mock and the assertion are self-consistently wrong. The correct mock value would be lastSnr: 6.0 with the fix applied (and the assertion expect(s.readInt16LE(42)).toBe(24) would still hold, since 6.0 * 4 = 24). The comment on the mock should say "dB" not "quarter-dB raw value".

🟡 Test: allowAdminCommands mismatch (test line 971–978)

it('relays status even when allowAdminCommands is off (read-only follow-up to login)', async () => {
  await startWith(true);   // ← starts with admin ON, contradicts the test description

The description says it should test the off case (the important one — proves it's ungated). It calls startWith(true) (admin on). This means the "off" case is never actually exercised. The fix is await startWith(false) (the happy-path test already covers allowAdminCommands: false, but this test was added to prove the ungated contract).

Compare the analogous test for SendLogin (line 748): await startWith(false); // ungated — correct pattern.

✅ What's well done

  • Codec structure: parseSendStatusReq is clean and correctly documented — no reserved bytes unlike SendTelemetryReq.
  • encodeStatusResponsePush wire layout ([0x87][reserved:1][pubKeyPrefix:6][statusData]) matches meshcore.js onStatusResponsePush.
  • encodeRepeaterStatusData field count and offsets — the 48-byte blob is correctly structured for the 16 fields meshcore.js reads in getStatus. The RepeaterStatusData interface comments cross-reference the JS names.
  • Relay pattern (Sent-ack first → async operation → push result) is consistent with the existing Login/TracePath/Telemetry handlers.
  • Ungated design — not gating on allowAdminCommands is correct for a read-only post-login operation.
  • Graceful failure — no push on null/throw lets the app time out rather than crashing.
  • Test coverage scope — the five new tests cover: happy path, ungated behavior, null result, manager-throws, and bad payload, which matches the SendTelemetryReq suite exactly.
  • Error handling — the Err(IllegalArg) path for short payloads is correctly tested and not gated by allowAdminCommands.
  • No performance concerns — the serialization is a single synchronous 48-byte buffer write.
  • No new security concerns — the publicKey extracted from the frame is passed to the manager as a hex string which the existing code already handles safely.

Summary

Issue Severity File Line
lastSnr encodes dB as wire units without ×4 scaling 🔴 Bug meshcoreCompanionCodec.ts 741
Test mock comment wrong ("quarter-dB raw" vs "dB") 🟡 Misleading meshcoreVirtualNodeServer.test.ts 89
Ungated test calls startWith(true) instead of false 🟡 Test gap meshcoreVirtualNodeServer.test.ts 972

The lastSnr bug will deliver a 4× underscaled SNR to the app on real hardware — worth fixing before merge, especially given this PR is explicitly NOT hardware-validated yet.

@Yeraze
Yeraze merged commit fc462d2 into main Jul 7, 2026
19 checks passed
Yeraze added a commit that referenced this pull request Jul 7, 2026
…cal node (#3904) (#3993)

Final gap of issue #3904: after login through the MeshCore Virtual Node, the
meshcore-flutter app's "neighbours" action fell through to `default:` in
`dispatchCommand` and returned Err(UnsupportedCmd).

Corrects the record: neighbours is NOT SendRawData(25) (the issue's inference).
It is SendBinaryReq(50) — a GENERIC binary-request command dispatched on an
inner sub-type — with sub-type GetNeighbours(0x06), answered by an async
BinaryResponse(0x8C) push. Verified against the vendored reference lib
`@liamcottle/meshcore.js` (`getNeighbours`, `sendCommandSendBinaryReq`,
`onBinaryResponsePush`, `sendBinaryRequest`), the SAME companion protocol
meshcore-flutter speaks.

Confirmed protocol:
- App→VN: `[50][targetPubkey:32][reqData]`; for neighbours
  `reqData = [0x06][request_version=0][count][offset:u16LE][order_by]`
  `[pubkey_prefix_len][random_tag:u32LE]`.
- VN→App: async BinaryResponse(0x8C) `[0x8C][reserved:1][tag:u32LE]`
  `[totalCount:u16LE][resultsCount:u16LE]` then resultsCount ×
  `[prefix:pubkey_prefix_len][heardSecondsAgo:u32LE][snr:int8=snr*4]`.
- Tag correlation: the client matches the push by `BinaryResponse.tag ==
  Sent.expectedAckCrc` (NOT the request's random_tag), so we echo the request's
  random_tag into BOTH the Sent's expectedAckCrc and the BinaryResponse tag.

Implementation (mirrors the #3961/#3991 Sent-ack → async-push template):
- `dispatchCommand` → `handleSendBinaryReq`: parse envelope, dispatch on the
  inner sub-type. GetNeighbours(0x06) implemented; any other sub-type gets an
  explicit Err(UnsupportedCmd) so future ones are obvious, not silently dropped.
- `handleGetNeighboursReq`: reply Sent (tag echoed), call
  `manager.getNeighbours(publicKey, {count, offset, orderBy})`, push a
  BinaryResponse re-serialized with the requested pubkey_prefix_len stride and
  snr as round(snr*4) int8.
- Ungated by allowAdminCommands — read-only follow-up to login, like
  SendTelemetryReq/SendStatusReq (the real node answers based on the session).
- Graceful: bad envelope/inner blob → Err(IllegalArg); null result (non-repeater
  / no session / disconnected) → empty BinaryResponse (client tolerates it);
  manager throw → log and emit nothing (app times out, matching siblings).

Codec: adds `parseSendBinaryReq`, `parseGetNeighboursReq`,
`encodeBinaryResponsePush`, `encodeNeighboursPayload`, `PushCodes.BinaryResponse
= 0x8C`, and `BinaryRequestTypes`.

VALIDATED END-TO-END against a live repeater: drove the real meshcore.js client
over TCP against the VN on :5000, handshook, and called
`getNeighbours('661b0674…')` — the VN relayed 16-total/10-returned real
neighbours from the physical node, tag-correlated (Sent expectedAckCrc ==
BinaryResponse tag). Tests mirror the sibling VN suites.


Claude-Session: https://claude.ai/code/session_015AFBA76hsjqhsXe1BdnYub

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant