fix: enable loopback transport for devnet and testnet modes

[mickvandijke]

Summary

• Set allow_loopback = true on CoreNodeConfig for Development and Testnet network modes in NodeBuilder::build_core_config()
• Set allow_loopback = true on CoreNodeConfig in Devnet::start_node() and TestNetwork::start_node()
• Uses the new NodeConfig.allow_loopback config field from saorsa-core instead of the removed SAORSA_TRANSPORT_ALLOW_LOOPBACK env var

Dependencies

• saorsa-core: feat: add allow_loopback config field to NodeConfig saorsa-labs/saorsa-core#37
• saorsa-transport: 95ab6720

Test plan

• cargo build passes
• cargo clippy clean
• cargo test — 254 passed, 1 pre-existing Anvil port conflict failure (unrelated)

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR fixes local devnet and testnet startup failures by ensuring SAORSA_TRANSPORT_ALLOW_LOOPBACK=true is set in the process environment before saorsa-core's transport layer initialises, allowing nodes bound to 127.0.0.1 to connect to each other. The constant is centralised in src/config.rs and consumed in three places: Devnet::start(), TestNetwork::start(), and NodeBuilder::build_core_config() (for Testnet and Development modes).

Key observations:

• std::env::set_var is called from async functions running on Tokio's multi-threaded executor. The Rust standard library documents that concurrent calls to set_var alongside any getenv from another thread cause undefined behaviour. While the actual risk here is low (the variable is always set to the same value before P2PNode::new is awaited), this is a correctness concern worth addressing — ideally by moving the call as early as possible or, better, by requesting a typed config field from saorsa-core.
• build_core_config is a synchronous helper that appears to only assemble a config struct, but it now silently mutates process-global state (the environment). This makes the function harder to unit-test in isolation and its side effects are not visible from the call site.
• For devnet.rs and testnet.rs, the placement (at the very top of start(), before node spawning) is reasonable and reduces the practical risk of the MT-safety issue.

Confidence Score: 3/5

• The PR is safe to merge for its stated goal (fixing loopback connectivity for devnet/testnet), but the use of std::env::set_var in async contexts introduces a latent MT-safety concern and a hidden side effect inside a config-builder function that should be addressed.
• The change is small and targeted. In practice the race window is narrow and the value being written is always the same, so real-world breakage is unlikely. However, embedding a process-global mutation inside build_core_config makes isolation tests for that function unreliable and violates the principle of least surprise, warranting a score below 4.
• src/node.rs — build_core_config has an undocumented process-environment side effect and calls set_var from an async execution context.

Important Files Changed

Filename	Overview
src/config.rs	Adds TRANSPORT_ALLOW_LOOPBACK_ENV constant with a clear doc comment — straightforward, no issues.
src/node.rs	Sets SAORSA_TRANSPORT_ALLOW_LOOPBACK=true for Testnet and Development modes inside build_core_config, a synchronous helper called from an async context; std::env::set_var is not MT-safe and buries a global side effect inside what appears to be a pure config builder.
src/devnet.rs	Calls std::env::set_var at the top of Devnet::start() before spawning nodes — placement is reasonable, but inherits the MT-safety concern of set_var in an async runtime.
tests/e2e/testnet.rs	Mirrors Devnet::start() pattern for TestNetwork::start(); same MT-safety concern but practically low-risk because the env var is set before any concurrent node startup.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Node Startup] --> B{Entry Point?}

    B -->|Devnet| C[Devnet::start]
    B -->|E2E Test| D[TestNetwork::start]
    B -->|Single Node| E[NodeBuilder::build]

    C --> F[set_var SAORSA_TRANSPORT_ALLOW_LOOPBACK=true]
    D --> G[set_var SAORSA_TRANSPORT_ALLOW_LOOPBACK=true]
    E --> H[build_core_config]

    H --> I{NetworkMode?}
    I -->|Production| J[No set_var]
    I -->|Testnet| K[set_var SAORSA_TRANSPORT_ALLOW_LOOPBACK=true]
    I -->|Development| L[set_var SAORSA_TRANSPORT_ALLOW_LOOPBACK=true]

    F --> M[spawn bootstrap nodes]
    G --> N[spawn test nodes]
    J --> O[P2PNode::new]
    K --> O
    L --> O

    M --> P[P2PNode::new per node]
    N --> P

    O --> Q[saorsa-core reads env var\nduring transport init]
    P --> Q

    Q --> R{Env var == true?}
    R -->|Yes| S[Accept 127.0.0.1 connections]
    R -->|No| T[Reject loopback connections]

Loading

Comments Outside Diff (1)

1. src/node.rs, line 174-241 (link)

   Env var side-effect inside a pure config-builder function

   build_core_config is a synchronous, pure-looking helper whose name and signature suggest it only builds and returns a CoreNodeConfig. Mutating the process environment inside it is a surprising side effect that:

   1. Makes the function hard to test in isolation (a test calling build_core_config with NetworkMode::Development will permanently alter the process environment for every subsequent test in that process, potentially affecting unrelated test cases).
   2. Hides the global-state mutation from callers who read only the function signature.

   Consider either documenting the side effect prominently in the doc comment, or moving the set_var call up to build() where side effects are already expected.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/node.rs
   Line: 174-241
   
   Comment:
   **Env var side-effect inside a pure config-builder function**
   
   `build_core_config` is a synchronous, pure-looking helper whose name and signature suggest it only builds and returns a `CoreNodeConfig`. Mutating the process environment inside it is a surprising side effect that:
   
   1. Makes the function hard to test in isolation (a test calling `build_core_config` with `NetworkMode::Development` will permanently alter the process environment for every subsequent test in that process, potentially affecting unrelated test cases).
   2. Hides the global-state mutation from callers who read only the function signature.
   
   Consider either documenting the side effect prominently in the doc comment, or moving the `set_var` call up to `build()` where side effects are already expected.
   
   How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 2b1cbbd}

[greptile-apps]

std::env::set_var is not thread-safe in an async/multithreaded context

std::env::set_var is documented to cause undefined behavior when called concurrently with any other environment-variable operation (including reads via getenv). Because NodeBuilder::build() is an async fn running on Tokio's multi-threaded executor, other OS threads (in saorsa-core, or system libraries) may be reading the environment at the same moment.

This is particularly risky here because build_core_config (a sync function) modifies the global environment as a side effect and is called right before P2PNode::new(core_config).await — meaning saorsa-core's transport layer could be reading the variable from a different OS thread during initialization.

The safest fix is to set the env var once at the very top of NodeBuilder::build() (before build_core_config is called) rather than burying it inside the match arms of build_core_config. Even better would be for saorsa-core to expose a typed config field for this instead of relying on a process-global env var.

The same issue exists in src/devnet.rs:394 and tests/e2e/testnet.rs:1062, where set_var is called inside an async fn that spawns multiple concurrent tasks shortly after.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/node.rs
Line: 210-233

Comment:
**`std::env::set_var` is not thread-safe in an async/multithreaded context**

`std::env::set_var` is documented to cause undefined behavior when called concurrently with any other environment-variable operation (including reads via `getenv`). Because `NodeBuilder::build()` is an `async fn` running on Tokio's multi-threaded executor, other OS threads (in saorsa-core, or system libraries) may be reading the environment at the same moment.

This is particularly risky here because `build_core_config` (a sync function) modifies the global environment as a side effect and is called right before `P2PNode::new(core_config).await` — meaning saorsa-core's transport layer could be reading the variable from a different OS thread during initialization.

The safest fix is to set the env var **once at the very top of `NodeBuilder::build()`** (before `build_core_config` is called) rather than burying it inside the `match` arms of `build_core_config`. Even better would be for saorsa-core to expose a typed config field for this instead of relying on a process-global env var.

The same issue exists in `src/devnet.rs:394` and `tests/e2e/testnet.rs:1062`, where `set_var` is called inside an `async fn` that spawns multiple concurrent tasks shortly after.

How can I resolve this? If you propose a fix, please make it concise.

[Copilot]

Pull request overview

This PR enables loopback (127.0.0.1) connections in saorsa-core’s transport layer for local dev/test networking by standardizing the environment variable name and setting it in devnet/testnet startup paths.

Changes:

• Added TRANSPORT_ALLOW_LOOPBACK_ENV constant (SAORSA_TRANSPORT_ALLOW_LOOPBACK) in src/config.rs.
• Set SAORSA_TRANSPORT_ALLOW_LOOPBACK=true when starting local Devnet and the E2E TestNetwork.
• Set the same env var during NodeBuilder::build_core_config() for Development and Testnet modes.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/config.rs	Introduces a shared constant for the transport loopback env var.
src/devnet.rs	Enables loopback acceptance before starting the in-process devnet nodes.
src/node.rs	Enables loopback acceptance for Development/Testnet NodeBuilder configuration.
tests/e2e/testnet.rs	Enables loopback acceptance before starting the E2E test network.

---

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

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

std::env::set_var here unconditionally overwrites any user-provided SAORSA_TRANSPORT_ALLOW_LOOPBACK value and applies process-wide. Consider only setting it when the env var is currently unset (so users can explicitly disable/override), and/or gate it on actually using loopback peers (e.g., any config.bootstrap addr is ip().is_loopback()).

[Copilot]

Setting SAORSA_TRANSPORT_ALLOW_LOOPBACK inside build_core_config has global side effects and currently overwrites any existing value. It’s safer to only set it if unset (and ideally restore the previous value if you later introduce multi-mode usage in one process).

[Copilot]

std::env::set_var changes process-wide state and is never restored/unset after the devnet is shut down. Since this devnet runs in-process, consider preserving any previous value and restoring it (or only setting when the env var is unset) to avoid leaking this setting into subsequent node runs in the same process.

Suggested change

	std::env::set_var(TRANSPORT_ALLOW_LOOPBACK_ENV, "true");
	if std::env::var_os(TRANSPORT_ALLOW_LOOPBACK_ENV).is_none() {
	std::env::set_var(TRANSPORT_ALLOW_LOOPBACK_ENV, "true");
	}

[Copilot]

This sets a global environment variable at runtime. If multiple tests in this integration test binary run in parallel, it can introduce cross-test coupling via shared process state. Consider setting it once behind a Once/OnceLock guard (and/or restoring the previous value when the test network stops) to keep tests isolated.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 7 changed files in this pull request and generated no new comments.

---

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

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.

---

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

You can also share your feedback on Copilot code review. Take the survey.