[feat][node] Add join stage-1 orchestration and primary test

[thep2p]

Summary

• Add join_stage1_link_level0, composing the three round-trip primitives from the earlier PRs in this stack: picks the search direction by comparing introducer.id() against this node's own id (Direction::Right when less, Direction::Left when greater, an error on collision), locates the search-resolved node and its neighbor, then sends the two independent GetLinkOp requests. The join also aborts when the search resolves a node holding this node's own id.
• Update the design doc (Section 3.2) to describe both directional branches, replacing an earlier version that only covered introducer.id() < u.id() and, as written, would have silently corrupted the graph's sorted-order invariant when a joining node's id is smaller than its introducer's.
• New primary test (test_join_stage1_link_level0_links_both_sides): the normal two-sided link case, asserting entries land on the correct side of this node's own table.

Part of #90's join stage-1 linking, split into a PR stack: the round-trip primitives (address, span-parentage, predecessor search, neighbor query, link request) are stacked as separate PRs below this one. Two more scenario tests (one-node-graph edge case, no-right-neighbor) plus a fourth (new-leftmost-node, covering the branch this PR's doc fix addresses) follow as small stacked PRs on top of this one.

Test plan

• cargo fmt --check
• cargo clippy --all-targets -- -D warnings
• cargo clippy --all-targets --all-features -- -D warnings
• cargo test

[Copilot]

Copilot review overview

🟡 Changes recommended

Two unresolved moderate issues affect typed collision handling and concurrent request dispatch.

Review effort: Lite
Findings: 2

Open (2)

• Collision errors are untyped and indistinguishable from transport failures · New
• Synchronous sends prevent concurrent link requests · New

What changed in this PR

Adds Stage 1 level-0 join orchestration, directional protocol documentation, and a two-sided linking test.

Changes:

• Selects search direction based on node identifiers.
• Discovers neighboring nodes and sends link requests.
• Updates protocol documentation and adds primary test coverage.

Review findings:

• Moderate: Collision errors are not surfaced through a dedicated typed error.
• Moderate: Link requests are not guaranteed to be dispatched concurrently.

File	Summary
src/​node/​base_node.rs	Implements join orchestration, error handling, and primary test coverage.
docs/​protocol/​concurrent-insert.md	Documents directional Stage 1 behavior and linking semantics.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[thep2p]

@claude review

[github-actions]

Claude finished @thep2p's task —— View job

---

Review: PR #139 — join stage-1 level-0 orchestration

• Read changed files (base_node.rs, concurrent-insert.md)
• Analyze correctness, error handling, tests
• Post review + verdict

I read both changed files in full and traced the orchestration against the design doc. This is a clean, well-documented composition of the existing round-trip primitives. Below are my findings.

Correctness — looks right

• Direction selection (base_node.rs:431-439): introducer < own_id → Right, > → Left, == → error. This matches Section 3.2 step 3, and the doc fix correctly closes the earlier "always search right" bug that would have corrupted sorted order for a new leftmost node.
• Two-sided linking (:464-489): search resolves the anchor on search_direction, the neighbor query reuses that same direction, and the two GetLinkOps go out on search_direction (to the anchor) and query_direction = opposite (to the neighbor). This lines up with the "important asymmetry" note in the doc (s-chain → left, z-chain → right).
• Collision aborts are both handled: introducer-id collision (:435) and search-resolves-own-id (:453), each with a covering test. Good — the foundOp case is easy to miss.
• None-neighbor branch (:482-488) leaves that side unresolved and only awaits search_link, consistent with Section 3.2/5.3 (background repair fills it later).

On the two Copilot findings — I agree with the author

• Untyped collision error: Given no caller branches on it today, a dedicated error variant would have no consumer, and the doc contract was updated to say "an error." Fixing the contract rather than adding a dead variant is the right call for this stack. Reasonable to revisit when a caller actually needs to distinguish it.
• tokio::join! vs. concurrent dispatch: The author is correct that protocol safety comes from each receiver's forwarding (Section 4), not from send overlap, and the doc no longer promises overlap. tokio::join! still overlaps on a real async transport; under MockNetwork the first send completes re-entrantly, which is a mock artifact, not a correctness issue. I agree a mock-ordering test would test the mock, not the protocol.

Minor observations (non-blocking)

1. send_search_by_id_req first-hop precondition (:442-451): its doc-comment (:187-191) requires dest <= target for Right / dest >= target for Left. Here target = own_id, dest = introducer, and the direction is chosen precisely so introducer < own_id ⇒ Right and introducer > own_id ⇒ Left, so the precondition holds by construction. Worth a one-line comment at the call site tying the direction choice to that precondition, since it's the invariant that makes the first hop valid — right now the reader has to reconstruct it.
2. max_level not seeded from get_max_level: join_stage1_link_level0 takes max_level as a parameter but nothing in this PR calls get_max_level to feed it. That's expected given the PR is one slice of a stack; just flagging that the phase-0 → phase-1 wiring lands elsewhere.
3. Test coverage: the primary two-sided test is thorough and the nonce-capture / yield_now delivery pattern is consistent with the sibling tests. The introducer > own_id (search-left) branch and the one-node-graph edge case aren't exercised here, but the PR body says those follow as stacked tests — reasonable to defer.

Note on verification

I was unable to run cargo test / cargo clippy in this environment (the sandbox blocked the invocation), so I relied on static review. The author lists fmt/clippy/test in the plan; CI should be the source of truth for those.

Overall this is solid work — the logic is correct, the error paths are covered, and the doc fix is a genuine improvement. Approving; the observations above are optional polish.

Job run · Branch: thep2p/90-join-stage1-orchestration

[github-actions]

Reviewed the stage-1 level-0 join orchestration and doc changes. The direction selection, two-sided linking, and both collision-abort paths are correct and match the design doc, and the doc fix genuinely closes the earlier new-leftmost-node ordering bug. I agree with the author's responses to both Copilot findings: a dedicated collision error would have no consumer today, and protocol safety comes from receiver-side forwarding rather than send overlap, so tokio::join! is fine. Minor non-blocking notes left in the PR comment (call-site precondition comment, deferred branch tests). Could not run cargo test/clippy locally, so CI is the source of truth for those. LGTM.

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation is covered, and the remaining documentation nit is non-blocking.

Review effort: Lite
Findings: None

Resolved since last review (2)

• Synchronous sends prevent concurrent link requests
• Collision errors are untyped and indistinguishable from transport failures

[thep2p]

Re: #139 (comment)

• Collision tests: only the search-resolves-own-id abort had a test. Added the introducer-collision test and the call-site comment: 18c0828
• max_level wiring: now in the scope of [Node] Implement: Stage-2 join and join entry point #92.
• Deferred tests: search-left, one-node-graph, and no-neighbor tests follow in the stacked PRs.
• CI: lint and test pass. CI never ran cargo fmt --check; [improve][ci] Check formatting in CI #150 adds it.