refactor: by-value Copy types, public fields, Nonce rename, internal/ policy

[TheP2P (thep2p)]

Summary

• Internal reference policy: moves skip-graphs-paper.pdf to internal/ (gitignored); adds rules to CLAUDE.md — no internal paths in source, restate content explicitly, agents follow the same rules.
• By-value for Copy types: Identifier and MembershipVector are 32-byte Copy types; methods now return and accept them by value throughout the codebase, eliminating unnecessary lifetime friction.
• Public fields on data structs: IdSearchReq and IdSearchRes have no invariants to protect — converted to plain structs with public fields, removing boilerplate getters and new() constructors. Callers use struct literal syntax.
• RequestId → Nonce: clearer name for a randomly generated token used once to correlate a response back to its originating request.

Test plan

• cargo test — all 58 tests pass, no regressions
• make lint — no clippy warnings

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR refactors several core skip-graph model types to be passed by value (for small Copy types), simplifies the search request/response API by using public fields, and renames RequestId to Nonce for clarity. It also updates tests and network/node plumbing accordingly, and adds an internal-reference policy to prevent leaking internal document paths into source code.

Changes:

• Switched Identifier / MembershipVector usage to by-value APIs and updated call sites.
• Refactored ID search request/response to plain structs with public fields and renamed RequestId → Nonce.
• Updated node/network code and tests to match the new signatures and struct-literal construction patterns.

Reviewed changes

Copilot reviewed 22 out of 25 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/test_debug_hex_format.rs	Updates tests to match by-value Identity construction and formatting style.
src/node/skip_graph_integration_test.rs	Updates integration tests to use Nonce and struct-literal IdSearchReq.
src/node/search_by_id_test.rs	Updates networking integration tests for IdSearchReq/Res public fields and Nonce.
src/node/core.rs	Changes Core trait APIs to by-value and updates search implementation to use public fields.
src/node/core_test.rs	Updates core tests to the new IdSearchReq/Res API and by-value IDs.
src/node/base_node.rs	Updates request correlation map to Nonce and refactors search/relay logic to by-value requests.
src/network/processor.rs	Formatting/import adjustments; keeps processor API consistent with by-value identifiers.
src/network/mod.rs	Minor import ordering and enum formatting cleanup.
src/network/mock/network.rs	Formatting/import adjustments in mock network implementation.
src/network/mock/network_test.rs	Updates tests for import ordering/formatting and minor cleanup.
src/lib.rs	Reorders module declarations.
src/core/testutil/fixtures.rs	Updates fixtures to construct identities by value; minor formatting refactors.
src/core/model/search.rs	Renames RequestId → Nonce; makes IdSearchReq/Res plain structs with public fields.
src/core/model/memvec.rs	Changes common_prefix_bit to take MembershipVector by value; updates tests.
src/core/model/identity.rs	Changes Identity::new, id(), and mem_vec() to by-value APIs.
src/core/model/direction.rs	Whitespace/formatting fix.
src/core/model/address.rs	Import ordering tweak.
src/core/lookup/mod.rs	Whitespace/formatting cleanup.
src/core/lookup/array_lookup_table.rs	Expands tracing call for readability.
src/core/lookup/array_lookup_table_test.rs	Import ordering and minor formatting cleanup.
src/core/context/mod.rs	Whitespace/formatting cleanup.
src/core/context/context_test.rs	Formatting cleanup in async tests.
CLAUDE.md	Adds “internal references” policy guidance.
.gitignore	Adds internal/ to gitignore with policy comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[TheP2P (thep2p)]

1b6b788

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[TheP2P (thep2p)]

Claude (@claude) perform a review for any critical flaws, code smells or bugs introduced in this PR.

[claude]

Claude finished TheP2P (@thep2p)'s task —— View job

---

Code Review

I reviewed the core changes for critical flaws, bugs, and code smells. Overall this is a clean, well-structured refactor — by-value Copy types, public-field data structs, and the Nonce rename all read well. Below are my findings, ordered by severity.

Critical / Correctness

1. base_node.rs:83-140 — originator's search_by_id blocks forever if the relayed node never responds.
After relaying, the originator calls rx.recv() (line 121), which blocks indefinitely. If the downstream node crashes, drops the request, or the response is lost, the SyncSender is never used and recv() never returns. There is no timeout. The nonce entry also stays in request_id_map forever in that scenario (the recv-error path on line 130 only fires when the sender is dropped, not when it's simply never sent). Recommend a recv_timeout with cleanup of the map entry on timeout. Not introduced by this PR per se, but the refactor is a good moment to flag it.

2. base_node.rs — no protection against nonce collision / pre-existing entry.
request_id_map.insert(req.nonce, tx) (line 103) silently overwrites any existing waiter for the same nonce. Nonce::random() makes collisions astronomically unlikely, but a caller could also reuse a Nonce (the struct is Copy and public). Overwriting drops the previous tx, which would unblock the earlier waiter with a recv error. Consider asserting/erroring on an occupied slot, or document that nonces must be unique per in-flight request.

Code Smells / Minor

3. base_node.rs:105-111 and :181-187 — duplicated IdSearchReq relay construction.
The originator and the forwarding path build an almost-identical relay IdSearchReq (nonce/target/level/direction copied, only origin differs). Since IdSearchReq is now Copy with public fields, this could be IdSearchReq { origin: ..., level: res.termination_level, ..req } to reduce duplication and avoid drift if a field is added later.

4. search.rs:6-8 — Nonce wraps a u128 but only exposes random().
There's no Eq-by-construction concern, but the field is private with no accessor; fine for now. Just note that Copy + public-struct IdSearchReq.nonce exposes the Nonce value but not its inner u128, which is the right encapsulation. No change needed.

5. core_test.rs:24-47 (re: Copilot's comment) — test_search_by_id_singleton_fallback uses level: 3, so it does still exercise level > 0 fallback on an empty table. Copilot's concern about weakened coverage appears to not apply to the current state of this file — the singleton fallback test uses level: 3, and test_search_by_id_not_found_left/right_direction iterate lvl across all levels with filled tables. Coverage looks intact.

Things that look correct / good

• The recv-error cleanup at base_node.rs:130-138 and the send-error cleanup at :113-119 correctly remove the nonce entry — this addresses the earlier Copilot leak comment (commit 1b6b788).
• By-value common_prefix_bit(other: MembershipVector) (memvec.rs:87) is appropriate for a 32-byte Copy type and all call sites pass by value cleanly.
• Identity by-value getters (identity.rs:22-34) are consistent with the Copy policy in CLAUDE.md.
• mutex was poisoned .expect() strings are lowercase per the style guide. ✅

Summary

No blocking bugs introduced by this PR. The most important issue is the unbounded blocking recv() in the originator path (#1), which is a robustness gap worth a follow-up (timeout + cleanup). #3 is a nice cleanup enabled by this very refactor. Copilot's core_test.rs coverage concern does not appear to hold against the current file.

---

• Read core model changes (search.rs, memvec.rs, identity.rs)
• Read node logic (base_node.rs, core.rs)
• Read network plumbing (processor.rs)
• Review tests for coverage regressions
• Post review findings
  • Branch: thep2p/search-by-memvec

[TheP2P (thep2p)]

1. base_node.rs:83-140 — originator's search_by_id blocks forever if the relayed node never responds.
   After relaying, the originator calls rx.recv() (line 121), which blocks indefinitely. If the downstream node crashes, drops the request, or the response is lost, the SyncSender is never used and recv() never returns. There is no timeout. The nonce entry also stays in request_id_map forever in that scenario (the recv-error path on line 130 only fires when the sender is dropped, not when it's simply never sent). Recommend a recv_timeout with cleanup of the map entry on timeout. Not introduced by this PR per se, but the refactor is a good moment to flag it.

Tracked in #74

2. base_node.rs — no protection against nonce collision / pre-existing entry.
   request_id_map.insert(req.nonce, tx) (line 103) silently overwrites any existing waiter for the same nonce. Nonce::random() makes collisions astronomically unlikely, but a caller could also reuse a Nonce (the struct is Copy and public). Overwriting drops the previous tx, which would unblock the earlier waiter with a recv error. Consider asserting/erroring on an occupied slot, or document that nonces must be unique per in-flight request.

As mentioned, astronomically unlikely, hence we avoid the overhead of adding belt-and-suspender for statistically improbable events.

3. base_node.rs:105-111 and :181-187 — duplicated IdSearchReq relay construction.
   The originator and the forwarding path build an almost-identical relay IdSearchReq (nonce/target/level/direction copied, only origin differs). Since IdSearchReq is now Copy with public fields, this could be IdSearchReq { origin: ..., level: res.termination_level, ..req } to reduce duplication and avoid drift if a field is added later.

Fixed in da13d00