refactor(registry)!: MeshCoreManager implements ISourceManager — one unified source registry (#3962)

[Yeraze]

Summary

Task 2.1 of the remediation plan (#3962, Phase 2) — the highest-leverage structural change of the phase: one registry, one source-manager interface, no parallel MeshCore world.

What changed

• MeshCoreManager implements ISourceManager: new sourceType getter, configure()/parameterless start()/stop() lifecycle (config staged on the instance), widened getStatus(sourceName?), setSourceName() (kept fresh on rename), getLocalNodeInfo() → null (documented — MeshCore has no nodeNum).
• One registry: meshcore sources now register in sourceManagerRegistry; all lifecycle (create/enable/disable/delete/config-change/connect/disconnect) and all ~30 read sites migrated. The load-bearing MeshCore quirk is preserved and now documented: manual /disconnect stops the manager but keeps it registered (routes 404 without it).
• Type-guard idiom: isMeshCoreManager()/isMeshtasticManager() in sourceManagerTypes.ts replace as (typeof meshtasticManager)[] casts; every meshtastic-assuming enumeration loop is hardened (security/dup-key/dead-node exclude meshcore; low-battery/inactive use the unified enumeration that replaces their explicit dual-registry merge).
• meshcoreRegistry.ts reduced to a @deprecated shim (delete after one release): get/list/remove delegate to the unified registry, disconnectAll faithfully scoped to meshcore-only; config helpers moved to meshcoreConfig.ts. Zero production callers remain.
• ensureMeshCoreManagerStarted() helper replaces the create-or-connect recipe that had been copy-pasted at 5 call sites.

Found and fixed by the adversarial review (8 finder angles, 11 verifiers)

Confirmed regression class — by-id lookups: pre-PR, getManager(<meshcore-id>) returned undefined and several Meshtastic-only paths leaned on that as an implicit guard. Post-unification a truthy MeshCoreManager reached sendTraceroute/sendRemoveNode/isNodeInDeviceDb calls → TypeError 500s (one reachable via attacker-controlled request body). Fixed at the shared-resolver altitude with isMeshtasticManager guards restoring exact pre-PR semantics: resolveSourceManager (covers v1 actions), purge-node, dead-nodes, bulk-delete, security scan — with regression tests.

Also from review: shim disconnectAll no longer tears down every source type; connect() keeps pendingConfig in sync; meshcoreRoutes guard now caches the narrowed manager in res.locals (kills a TOCTOU + honors the type-guard rule); touched handlers converted to the fail() envelope; log/doc accuracy nits.

Behavior notes

Behavior-preserving by design, verified per-site. Deliberate calls: VN status stays meshtastic-filtered; security-class scans keep excluding meshcore; graceful-shutdown semantics untouched (follow-up filed).

Verification

• Full suite (independent orchestrator run, post-fixes): 8266 passed, 0 failed, 0 suite failures (success: true). Typecheck clean; test-typecheck 282 (one below the 283 baseline); npm run lint:ci green.
• New test files: interface/guard truth tables, unified-registry lifecycle + mixed-protocol enumeration + manual-disconnect invariant (47 tests), shim behavior, by-id regression tests.

Refs #3962

🤖 Generated with Claude Code

https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: MeshCoreManager implements ISourceManager — unified registry (#3962)

Todo List:

• Gather context and understand the PR scope
• Review core interface and type changes (sourceManagerTypes.ts, meshcoreManager.ts)
• Review registry changes (meshcoreRegistry.ts, sourceManagerRegistry.meshcore.test.ts)
• Review route handler changes (security, meshcore, source routes)
• Review resolveSourceManager.ts changes and regression tests
• Review test coverage and quality
• Post comprehensive review feedback

---

Summary

This is a high-quality, well-executed structural refactor. The adversarial review process the PR describes is clearly visible in the result — regression tests exist for the specific TOCTOU and type-safety bugs that unification would have introduced. The new unified registry approach is architecturally sound. My comments below are mostly minor quality observations and a couple of things worth discussing.

---

Observations

1. isMeshtasticManager covers only meshtastic_tcp — MQTT sources are excluded

src/server/sourceManagerTypes.ts:34

export function isMeshtasticManager(m: ISourceManager): m is MeshtasticManager {
  return m.sourceType === 'meshtastic_tcp';
}

MQTT bridge/broker sources (mqtt_broker, mqtt_bridge) use MeshtasticManager instances too (they appear in resolveSourceManager and buildMqttManagerForSource). The guard returning false for those types means code that filters with isMeshtasticManager will silently skip active MQTT sources. The securityRoutes.ts dead-nodes filter at line 376 (m.sourceType !== 'meshcore') avoids this issue by negation, but it's inconsistent with the "use the guard predicates" rule in CLAUDE.md.

This might be intentional (MQTT sources perhaps do NOT implement the full MeshtasticManager interface), but it warrants a comment in sourceManagerTypes.ts to make the distinction explicit — especially since the CLAUDE.md isMeshtasticManager usage example on the getAllManagers().filter(isMeshtasticManager) line would silently exclude MQTT sources from enumeration results.

2. ensureMeshCoreManagerStarted silently handles non-MeshCore registered manager

src/server/meshcoreConfig.ts:79

} else if (isMeshCoreManager(existing) && !existing.isConnected()) {
    await existing.connect(cfg);
} else {
    logger.debug(`... Manager already registered as meshcore and connected — skipping auto-connect`);
}

The else branch covers three cases: (a) already registered as meshcore AND connected, (b) registered but NOT as meshcore. Case (b) would silently log "already connected" when the sourceId is registered as a different manager type — a defensive error log or early fail for the type-mismatch sub-case would aid debugging. Low risk in practice since source types are immutable after creation, but worth noting.

3. resolveSourceManager comment says "60+ handlers" but the function exists in the new PR

src/server/utils/resolveSourceManager.ts:16

"Centralizes the inline pattern that previously appeared in 60+ handlers"

The resolveSourceManager.ts function predates this PR (it's listed as MODIFIED, not ADDED). The comment is unchanged from before, so this is pre-existing, but the PR description says "~30 read sites migrated." The comment in the file doesn't match either count. Minor doc nit.

4. meshcoreRegistry.ts shim: disconnectAll calls removeManager — semantics mismatch

src/server/meshcoreRegistry.ts:51-56

disconnectAll(): Promise<void> {
    const meshcoreManagers = sourceManagerRegistry.getAllManagers().filter(isMeshCoreManager);
    return Promise.allSettled(
      meshcoreManagers.map(m => sourceManagerRegistry.removeManager(m.sourceId))
    ).then(() => undefined);
}

The original disconnectAll in the pre-PR registry called disconnect() (keeping the manager registered). This shim now calls removeManager() (which removes them). The deprecation note says "Note: this scopes to meshcore managers only — it does NOT remove meshtastic sources," but doesn't document the semantics change from disconnect→remove. Since this is a @deprecated shim with zero production callers, it's not a live bug, but callers who relied on the old behavior (if any existed) would behave differently. The deprecation javadoc could mention this divergence.

5. securityRoutes.ts — inconsistent response envelope in the /dead-nodes path

src/server/routes/securityRoutes.ts:421

res.json({ nodes: deadNodes, count: deadNodes.count, thresholdDays: DEAD_NODE_DAYS });

vs. the error paths in the same function that correctly use fail(). The success response on line 421 uses a bare res.json — no ok() envelope. This is consistent with how the route worked before (it's pre-existing), but since the PR explicitly touches this file and converts some handlers to fail(), it's a good opportunity to flag for the next pass.

6. sourceRoutes.ts — large if-else if chain on enable/disable transitions

src/server/routes/sourceRoutes.ts:531–700 (approximately)

The PUT /:id handler has grown a long if-else if chain, one branch per (sourceType × transition) combination. The MeshCore branches added in this PR integrate cleanly with the existing structure, but this chain is now 10+ branches and will need a source-type dispatch table to remain maintainable. Not a PR blocker, but a follow-up refactor candidate.

7. Test coverage for ensureMeshCoreManagerStarted

src/server/meshcoreConfig.test.ts covers meshcoreConfigFromSource and virtualNodeConfigFromSource well, but ensureMeshCoreManagerStarted is not tested in that file. The WP1 test (sourceManagerRegistry.meshcore.test.ts) covers registry lifecycle but doesn't call ensureMeshCoreManagerStarted directly. The function has 3 branches (no manager, disconnected manager, connected manager) and the second and third aren't exercised with unit tests. This is a gap worth filling, especially the "registered but non-MeshCore type" edge case from observation #2 above.

---

Strengths

• Type-guard approach (isMeshCoreManager/isMeshtasticManager based on sourceType discriminant) is clean and avoids import cycles. The leaf-module placement is correct.
• res.locals.meshcoreManager caching in the router-level guard eliminates the TOCTOU window and is exactly the right fix for the narrowing-then-lookup pattern.
• @deprecated shim is well-documented and the getOrCreate hard-throw is the right way to make callers migrate.
• Security scan, dead-nodes, and bulk-delete all have isMeshtasticManager guards with appropriate INVALID_SOURCE_TYPE error codes and regression tests.
• setSourceName() on rename is a nice quality-of-life fix that preserves status accuracy without a restart.
• Test file sourceManagerRegistry.meshcore.test.ts covers all 5 invariants including the load-bearing manual-disconnect behavior.

---

Minor / Nits

• sourceManagerTypes.test.ts line 83: isMeshCoreManager(makeStub('mqtt_broker')) || isMeshtasticManager(makeStub('mqtt_broker')) — the claim is "mqtt types are not identified by either guard". True, but the test would be stronger if it also asserted that MQTT manager instances actually work in the registry (since MqttBrokerManager/MqttBridgeManager presumably implement ISourceManager but have sourceType !== 'meshtastic_tcp'). Cosmetic concern.
• The sourceRoutes.ts MQTT link hot-swap at line 660 still uses as MeshtasticManager | undefined cast instead of isMeshtasticManager. Pre-existing, but since the file is touched this is a clean-up opportunity.

---

Verdict

Approve with the above observations noted. The regression fixes (type guards on security/dead-node/scan routes) are correct and tested. The architectural change (unified registry) is well-executed. The observations above are either pre-existing, documentation-only, or low-priority follow-ups that don't affect correctness of this PR.