Claude finished @Yeraze's task —— View job

PR Review: fix(meshcore): refresh known-but-nameless contacts (#3820)

Tasks:

• Gather context and understand the request
• Read meshcoreManager.ts changes
• Read test file changes
• Post review feedback

Summary

This is a clean, well-reasoned fix. The root cause diagnosis is thorough, the change is surgical, and the test coverage is solid.

Code Quality

src/server/meshcoreManager.ts (lines ~1167–1195)

The old logic:

if (!wasKnown) {
  void this.notifyNewNodeDiscovered(updated);
}
// ... schedule refresh only for new nodes (implicitly)

The new logic correctly separates the "notify of new discovery" concern from the "refresh if name/type is missing" concern:

if (!wasKnown) {
  void this.notifyNewNodeDiscovered(updated);
}
// refresh fires for known-but-nameless contacts too
if (!updated.advName || updated.advType === undefined) {
  this.schedulePathRefresh(publicKey);
}

This is the right separation. The two concerns were conflated before, and the fix correctly decouples them.

The comment block at lines ~1175–1192 is excellent — it documents the firmware behavior assumptions, the lastAdvert≈0 reliance, and flags the specific future risk precisely. This level of documentation for a non-obvious firmware interaction is exactly what's needed here.

Potential Issues

1. advType === undefined check (minor)

The condition !updated.advName || updated.advType === undefined uses strict undefined equality for advType. This is correct given the spread pattern (advType: data.adv_type ?? existing.advType), but worth noting that if advType is ever null (e.g., from a DB round-trip), this guard would not catch it. Probably not a real concern given the in-memory contact map, but worth being aware of.

2. Re-triggering on every nameless advert (acceptable trade-off)

Until refreshContacts() completes and updates this.contacts, a flurry of pubkey-only adverts within the debounce window could all evaluate !updated.advName as true. The debounce in schedulePathRefresh correctly coalesces these into a single get_contacts call, so this is not a bug — just confirming the existing debounce mechanism handles it correctly (it does).

3. The lastAdvert≈0 dependency (acknowledged)

The PR correctly calls out this assumption in a NOTE comment. The risk is real: if discovery ever stores a non-zero lastAdvert, the firmware's replay guard would suppress the subsequent advert, contact_advertised would never fire, and the name would never populate. No action needed here — it's documented and flagged appropriately.

Test Coverage

The new test case at line 139 ('refreshes an already-known but NAMELESS contact so its name populates (#3820)') directly exercises the regression:

• Seeds a known-but-nameless stub in this.contacts
• Fires a pubkey-only contact_advertised event
• Asserts exactly one get_contacts call fires after the debounce
• Asserts the name and type populate correctly

This is a precise regression test. The existing three tests remain intact (new node with only pubkey, complete advert skips refresh, already-known-and-named skips refresh), ensuring no regression in those paths.

One minor gap: there's no test verifying that a known-but-nameless contact receiving multiple pubkey-only adverts within the debounce window still results in exactly one get_contacts call (coalescing). This is covered implicitly by the debounce mechanism, but an explicit test would be belt-and-suspenders. Not blocking.

Verdict

Approve. The fix is correct, minimal, and well-documented. Test coverage directly addresses the regression. The lastAdvert≈0 assumption is explicitly called out in code comments. No blocking issues.

Claude finished @Yeraze's task —— View job

PR Review: fix(meshcore): refresh known-but-nameless contacts (#3820)

Tasks:

• Gather context and understand the request
• Read meshcoreManager.ts changes
• Read test file changes
• Read meshcoreNativeBackend.ts changes
• Read meshcoreRoutes changes
• Post review feedback

Summary

A clean, well-scoped fix with two complementary strategies: a passive path (refresh on advert for known-but-nameless contacts) and an active path (ANON_REQ OWNER during discovery). The root cause analysis is thorough and accurate. The code quality is high overall.

Core Fix (meshcoreManager.ts lines 1167–1195)

Correct separation of concerns. The !wasKnown gate for notifyNewNodeDiscovered is correctly preserved, while the name/type refresh trigger is correctly decoupled from it:

if (!wasKnown) {
  void this.notifyNewNodeDiscovered(updated);
}
if (!updated.advName || updated.advType === undefined) {
  this.schedulePathRefresh(publicKey);
}

The comment block (lines 1176–1192) is excellent — it documents the firmware relay behavior, the lastAdvert≈0 reliance, and flags the specific future risk precisely.

Minor observation: The advType === undefined check uses strict equality. If advType ever comes back as null from a DB round-trip and gets spread into existing, this guard won't catch it. The in-memory contact map makes this unlikely in practice, but worth being aware of.

Active Name Fetch (fetchOwnerName + request_owner)

The fetchOwnerName method is well-structured and defensive:

• Correctly installs a zero-hop direct out_path first (firmware gates OWNER response on isRouteDirect())
• Best-effort: swallows all failures with a debug log, keeping the passive advert path as fallback
• Response parsing strips non-printable chars as defense against stray binary payloads

Potential issue — request_owner and BinaryResponse listener:

c.on(K.PushCodes.BinaryResponse, onResp);  // persistent until cleanup

This uses c.on() (not c.once()), which is correct since a Sent must arrive first. But the onResp guard only filters on sentReceived:

const onResp = (r: any) => {
  if (!sentReceived) return;  // only after Sent
  cleanup();
  resolve(r?.responseData ?? new Uint8Array());
};

This is the same pattern as request_regions and is correct for this serialized (exclusive radio op) context. ✓

Potential concern — no tag matching on BinaryResponse. The request_owner handler accepts the first BinaryResponse that arrives after Sent, with no tag validation (same as request_regions). The runExclusiveRadioOp serialization prevents this from being a real bug in the normal case, but if another push (e.g. a telemetry reply from a different node) arrives between Sent and the OWNER reply, it could be misidentified. This is an inherent limitation of the tagless ANON_REQ protocol and is called out in the existing comment. ✓

Discovery Integration (discoverNodes + route)

The fetchNames opt-in parameter (default false) is the right design choice:

• Internal calls (region discovery sweep) don't pay the per-node OWNER fetch cost
• User-facing "Discover" action opts in with fetchNames=true

The sequential await loop at lines 2606–2614 is intentionally serial (one ANON_REQ at a time, matching the exclusive radio op constraint). This is correct.

The guard isRepeaterish && !contact?.advName (line 2611) is correct — only fetches names for nameless repeaters/room servers. Chat nodes don't support handleAnonOwnerReq, so they'd time out at 15s each. Good that they're excluded.

Test Coverage

meshcoreManager.newNodeRefresh.test.ts — The new test at line 139 directly exercises the regression:

• Seeds a known-but-nameless stub
• Fires a pubkey-only advert event
• Asserts exactly one get_contacts fires after debounce
• Asserts name and type populate correctly ✓

meshcoreManager.ownerName.test.ts — Three solid cases:

• Happy path: set_out_path before request_owner, name applied to contact ✓
• Failure path: owner request fails, contact stays unnamed, no throw ✓
• Non-companion guard: returns null immediately without sending any commands ✓

Gap (minor, non-blocking): No test verifying the isRepeaterish && !contact?.advName guard in discoverNodes — i.e., that a chat node discovered alongside a repeater doesn't get a fetchOwnerName call (which would time out on real hardware). The route test mocks discoverNodes wholesale, so this path isn't exercised. The logic is simple enough that it's low risk, but worth a note.

meshcoreRoutes.test.ts Changes

The mock update adding fetchOwnerName to the stub manager is correct and necessary. No issues.

Verdict

Approve. The fix is correct, minimal, and well-documented. The two-path strategy (passive advert refresh + active ANON_REQ) is appropriately layered. Test coverage directly addresses the regression. No blocking issues.