feat(neighbor-graph): treat ANON_REQ as an originator↔path[0] edge source (#1777)

[Saarlandpower]

Addresses #1777 — implements suggestion #1 only (extend ADVERT-only originator-edge logic to also accept ANON_REQ). Suggestion #2 (treat REQ/RESP/PATH/TXT's 1-byte src/dst hash as a hop) was explicitly rejected in the issue thread by both the meshcore-reviewer and the reporter themselves — ~1/256 collision odds, would manufacture false edges.

What

ANON_REQ (payload type 0x07) carries the sender's full Ed25519 ephemeral pubkey (decoder.go's EphemeralPubKey / ephemeralPubKey JSON field) — the same trust level as an ADVERT's pubKey. Both places that build the originator↔path[0] edge only recognized ADVERT:

• cmd/ingestor/neighbor_builder.go:209 (persisted neighbor_edges, read by the server)
• cmd/server/neighbor_graph.go:296 / extractFromNode (in-memory mirror, BuildFromStore)

Both now also accept ANON_REQ via a new hasFullOriginator := isAdvert || isAnonReq gate. REQ/RESP/PATH/TXT remain excluded from originator-edge creation — their existing observer↔path[last] edge (unaffected by this change, already applies to all packet types) still applies.

Changes

• cmd/ingestor/maintenance.go — new extractPubkeyFromAnonReqJSON, mirroring the existing extractPubkeyFromAdvertJSON.
• cmd/ingestor/neighbor_builder.go — payloadAnonReq const; falls back to the new JSON helper when from_pubkey is empty (that column is only populated for ADVERT at write time, per db.go's bug: pubkey attribution via LIKE-on-JSON is unsound — adversarial spoofing + same-name false positives + perf #1143 comment — left untouched to keep this change scoped to the two call sites triage identified).
• cmd/server/neighbor_graph.go — extractFromNode also checks "ephemeralPubKey" (safe unconditionally: the field only appears in ANON_REQ's decoded_json, via the shared Payload struct's omitempty).

Tests

• neighbor_graph_test.go: TestBuildNeighborGraph_AnonReqSingleHopPath (ANON_REQ produces the edge) + TestBuildNeighborGraph_ReqRespStillExcluded (REQ still doesn't).
• neighbor_builder_test.go: same two cases against the persisted-DB path (TestNeighborEdgesBuilderUpsertsFromAnonReqEphemeralPubKey, TestNeighborEdgesBuilderExcludesOtherNonAdvertTypes), exercising the from_pubkey-empty JSON fallback specifically.
• All existing neighbor-graph/-builder tests pass unchanged, including TestBuildNeighborGraph_ADVERTOnlyConstraint.
• cmd/server full suite: pass, except 2 pre-existing flaky tests (TestHandleNodePaths_*_1352, an index-loading race unrelated to this change — reproduces intermittently on unmodified master too, confirmed separately).
• cmd/ingestor full suite: pass.

Operator context: SaarMesh (SaarLorLux, DE/FR/LU, 800+ nodes) — more originator edges from ANON_REQ traffic should sharpen the neighbor graph / topology views for us.

Co-Authored-By: Claude noreply@anthropic.com

[Kpa-clawbot]

Bot polish review

Verdict: LGTM w/ nits
3-axis: mergeable=MERGEABLE · CI=pending (no checks reported yet) · findings=0 BLOCKER, 0 MAJOR, 2 minor

Findings

• minor munger: dead-code drift. cmd/server/neighbor_persist.go:247 (extractEdgesFromObs) still gates on isAdvert only. It has no live callers today (grep confirms), but leaving it inconsistent with the two paths this PR updates invites a future regression when someone revives it. Either delete it or apply the same hasFullOriginator gate.
• minor carmack: extractFromNode field priority. Adding "ephemeralPubKey" to the loop is safe (firmware decoder.go:109,471,525 confirms it's only populated on PayloadANON_REQ), but its position between pubKey and from_node/from is load-bearing on the omitempty invariant. Worth a one-line assert or godoc note that the field is invariant-scoped to ANON_REQ so no future decoder ever populates it on another type.

Test discipline

Red-commit not separated (single squash commit), but both new tests fail on revert of the hasFullOriginator gate — real assertions (edge count / row count), not tautologies. Boundary test (ReqRespStillExcluded / ExcludesOtherNonAdvertTypes) correctly locks the scope. Kent-beck: acceptable for a community drive-by PR.

Nits (optional, ≤3)

• Firmware assumption verified: firmware/src/Mesh.cpp:497-518 createAnonDatagram writes sender.pub_key (full 32-byte pubkey) into ANON_REQ payload — matches the PR's trust claim.
• CI status not yet reported; recommend waiting for green before merge.
• payloadAnonReq = 0x07 in ingestor duplicates PayloadANON_REQ in server; shared const across packages would be cleaner but out of scope.

[Saarlandpower]

Addressed both nits from the round-1 review:

1. (munger) Dead code: extractEdgesFromObs/edgeCandidate in neighbor_persist.go had zero callers anywhere (verified via grep, including tests) — its doc comment claiming BuildFromStore calls it was already stale. Deleted rather than duplicated the ANON_REQ fix into unreferenced code, per your drift-risk concern.
2. (carmack) Field-presence safety: extractFromNode's ephemeralPubKey lookup is now gated on tx.PayloadType == PayloadANON_REQ instead of relying solely on the JSON key being present, so it stays correct even if a future decoder change reuses that field name elsewhere.

No behavior change — all existing + #1777 tests pass unchanged. Full cmd/server suite clean on 2 runs.

Co-Authored-By: Claude noreply@anthropic.com

[Kpa-clawbot]

Bot polish re-review (round 2)

Verdict: LGTM — round-1 nits addressed

Delta since round-1 (1f2ed0c → 56d5a49)

• munger nit resolved: extractEdgesFromObs + edgeCandidate deleted from cmd/server/neighbor_persist.go. Grep confirms zero callers in tree post-delete. Stale doc-comment referencing neighbor_graph.go is gone with it. No drift risk remaining.
• carmack nit resolved: extractFromNode in cmd/server/neighbor_graph.go:374 now gates ephemeralPubKey lookup on *tx.PayloadType == PayloadANON_REQ instead of trusting field-name presence. Godoc explains the invariant (lines 365–373). Correct scope.
• Ingestor path (cmd/ingestor/neighbor_builder.go) unchanged from round-1 — already payload-type-gated via isAnonReq.

3-axis

• mergeable=MERGEABLE, mergeStateStatus=BLOCKED (CI action_required — awaits operator workflow approval for community author)
• findings: 0 BLOCKER, 0 MAJOR, 0 minor
• tests: TestNeighborEdgesBuilderUpsertsFromAnonReqEphemeralPubKey + TestNeighborEdgesBuilderExcludesOtherNonAdvertTypes still fail-on-revert (real assertions on edge count / row count). Scope boundary locked.

Merge gate

Ready pending CI approval + green. Not auto-mergeable from bot side (community branch, workflow approval required).

[Kpa-clawbot]

Bot polish re-review (round 3 — merge-from-master)

Verdict: LGTM (unchanged from round-2)

Delta since round-2 (56d5a49 → 6ea20de)

Sole new commit is a clean merge of origin/master. git diff 56d5a49 6ea20de -- cmd/ingestor/maintenance.go cmd/ingestor/neighbor_builder.go cmd/ingestor/neighbor_builder_test.go cmd/server/neighbor_graph.go cmd/server/neighbor_graph_test.go cmd/server/neighbor_persist.go returns empty — zero drift in PR-scope files. Merge pulled in unrelated master work (repeater_liveness.go #1755-era, observer↔node cross-nav #1826, unscoped_relay_count_24h #1823) with no textual conflict.

Personas (parallel)

• meshcore: protocol claim (ANON_REQ payload type 0x07 carries full 32-byte Ed25519 pubkey, same trust as ADVERT) still holds — firmware/src/Mesh.cpp createAnonDatagram writes sender.pub_key. No firmware churn on master relevant here.
• munger: no new spoofing surface. hasFullOriginator gate untouched; edge-poisoning risk unchanged from LGTM at round-2. Truncated-hash rejection for REQ/RESP/PATH/TXT remains scoped-out per issue triage.
• carmack: neighbor-graph merge path perf unchanged (no code delta). ANON_REQ hits the same dedup as ADVERT in neighbor_edges upserts.
• kent-beck: red-commit still verifiable — TestNeighborEdgesBuilderUpsertsFromAnonReqEphemeralPubKey and TestBuildNeighborGraph_AnonReqSingleHopPath fail-on-revert of hasFullOriginator; boundary tests (ReqRespStillExcluded, ExcludesOtherNonAdvertTypes) locked. No new tests required for a null-diff merge.

3-axis

• mergeable=MERGEABLE, mergeStateStatus=BLOCKED (CI action_required — community-author workflow-approval still pending; gh pr checks 1822 reports "no checks reported")
• findings: 0 BLOCKER, 0 MAJOR, 0 minor (unchanged from round-2)
• author owes nothing further

Merge gate

Still gated only on operator approving the CI workflow run + green. Bot side clean.

[SaarMesh-Bot]

Bot review has been green for a while now — this looks ready whenever a maintainer has a moment to approve the CI run (action_required gate). Let me know if anything else is needed on my end. Thanks!

[Kpa-clawbot]

Bot polish re-review (round 4 — post-master-merge)

Verdict: LGTM (unchanged from round-3)

Delta since round-3 (6ea20de → 204ce19)

Sole new commit is a clean merge from master; the six files in scope (cmd/ingestor/{maintenance,neighbor_builder,neighbor_builder_test}.go, cmd/server/{neighbor_graph,neighbor_graph_test,neighbor_persist}.go) are byte-identical vs round-3. No re-review of substance needed.

Parallel-persona check on the full diff (fresh cold read)

• meshcore — payload type 0x07 confirmed at cmd/server/decoder.go:34; EphemeralPubKey decoded as hex.EncodeToString(buf[1:33]) (decoder.go:471), 32-byte Ed25519 pubkey — same trust class as ADVERT pubKey. Exclusion of REQ/RESP/PATH/TXT (1-byte truncated src/dst) matches the feat: Treat SRC-ID as Hop #1777 discussion. Local payloadAnonReq = 0x07 mirror in cmd/ingestor/neighbor_builder.go is acceptable (module-boundary duplication, documented).
• carmack — hasFullOriginator computed once per tx; server path uses cached ParsedDecoded(); ingestor JSON fallback only fires when from_pubkey empty AND ANON_REQ. No hot-path regressions.
• munger — self-edge guards and prefix-uniqueness gate preserved on both surfaces. Ingestor↔server symmetry intact. Round-2 dead-code cleanup (extractEdgesFromObs) confirmed dropped.
• kent-beck — 4 tests, both surfaces × (accept + reject). Accept tests fail on revert (verified by inspection of the found assertions). Minor: negation tests only exercise REQ (0x02); parameterising over RESP/PATH/TXT would harden defense-in-depth but the code branch is unambiguous — not a blocker.

3-axis

mergeable=MERGEABLE · mergeState=BLOCKED (awaiting maintainer action_required gate on CI) · findings=0 BLOCKER, 0 MAJOR, 0 must-fix, 1 nit (deferred)

Ready for maintainer approval on the workflow run.

[Kpa-clawbot]

PR #1822 Review — feat(neighbor-graph): treat ANON_REQ as originator↔path[0] edge source

0 BLOCKER · 0 MAJOR · 3 MINOR

Protocol correctness (meshcore-persona)

Model is sound. firmware/src/Packet.h:26 confirms ANON_REQ (0x07) carries a full ephemeral Ed25519 pubkey — same trust tier as ADVERT's pubKey. The explicit exclusion of REQ/RESP/PATH/TXT (1-byte truncated hash, ~1/256 collision) is correct per the firmware packet layout and the #1777 discussion consensus. ✅

Findings

[MINOR · carmack] extractPubkeyFromAnonReqJSON (maintenance.go:254–272) duplicates the structure of extractPubkeyFromAdvertJSON (maintenance.go:240–252) — identical JSON-parse-then-field-extract pattern, differing only in the key name. A single extractPubkeyFieldFromJSON(s, fieldName string) helper would eliminate the duplication. Not blocking because the two functions are small and stable, but worth a follow-up.

[MINOR · dijkstra] payloadAnonReq in neighbor_builder.go:40 uses camelCase (payloadAnonReq) while the server package uses PayloadANON_REQ (decoder.go:34). The naming inconsistency is deliberate (unexported vs exported), but the casing style differs (AnonReq vs ANON_REQ). Consider payloadANON_REQ for grep-ability across packages.

[MINOR · kent-beck] Tests are well-structured — positive case (ANON_REQ produces edge) and negative case (REQ excluded) on both code paths (in-memory graph + persisted builder). The builder test correctly exercises the from_pubkey-empty JSON-fallback path. No TDD concern. One nit: TestNeighborEdgesBuilderExcludesOtherNonAdvertTypes could also assert the observer↔path[last] edge is still created for REQ (proving only the originator edge is suppressed, not all edges).

Dead code removal (commit 2)

Deletion of extractEdgesFromObs / edgeCandidate from neighbor_persist.go is correct — grep confirms zero callers. Removing stale parallel logic that would silently miss ANON_REQ if re-wired is the right call. (munger: approve)

Verdict

Clean, well-scoped community contribution. Protocol model matches firmware. Tests cover both positive and negative cases on both paths. All minors are follow-up candidates, none blocking.

[SaarMesh-Bot]

Superseded by #1863.

That PR carries both commits from this branch unchanged and adds the #1784 path-trust gate to the same function (buildAndPersistNeighborEdges). Keeping them apart would have left two open PRs rewriting the same code from two different accounts of mine, so they land together instead.

Closing here — review continues in #1863.