fix(sns-cli): resolve shared local network config in vendored dfx-core - #10914
Merged
Conversation
resolve_local_network() in rs/sns/dfx-core-vendored/src/network.rs decided "project-scoped local network" vs. "shared local network" purely by whether any dfx.json existed above the working directory, without checking whether that dfx.json actually declares a networks.local entry. Projects whose dfx.json has no networks key at all (e.g. snsdemo's) were wrongly treated as project-scoped and given the hardcoded 127.0.0.1:8000 default, instead of falling back to the shared network and reading its configured bind from ~/.config/dfx/networks.json (which can be customized, e.g. 127.0.0.1:8080). This mirrors dfx-core's own create_project_network_descriptor / create_shared_network_descriptor split: a project's dfx.json only wins for a network it actually declares; otherwise dfx (and now this vendored subset) falls back to the shared networks.json, defaulting to 127.0.0.1:4943 only when that file has no local entry either. Adds unit tests covering: project dfx.json without a networks key falling back to the shared network's configured bind; a project dfx.json that does declare its own local network taking precedence; and the shared-network default applying when neither config declares local. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi
Per Daniel Wong's review on #10914: - Rewrite convoluted doc comments (module doc, find_project_local_network, shared_local_address) using short declarative sentences. - Restructure find_project_local_network as a guard-clause loop: the "no dfx.json here, try the parent" case returns/continues early, and the "found dfx.json" case is the fallthrough with least indentation. - Replace and_then/map combinator chains for reading dfx.json and networks.json with explicit match statements and early return. - Distinguish "dfx.json/networks.json absent or has no local network" (expected, falls through to None/default) from "present but malformed JSON, or bind is present with the wrong type" (a real error, now surfaced via new NetworkResolutionError variants instead of being silently treated the same as "not found"). - Restructure the "bind" lookup as two statements (look up the key, then decide what to do with it) instead of one long dot chain. - Move the test module into its own network_tests.rs file, referenced via #[path = "network_tests.rs"], and hoist the DFX_CONFIG_ROOT use to the top of that file. - Use distinctive, non-coincidental bind values in test fixtures/asserts (e.g. "shared:2718", "dfx-json:9999") so a passing test can't be an accident; strengthen the precedence test to also configure a distinct shared network; and change the "no local network" test to use a dfx.json/networks.json that exist but don't declare local, instead of files that don't exist at all. - Drop the stale "regression test" comment and the post-assert manual temp-dir cleanup (redundant, and skipped whenever the assert fails). - BUILD.bazel: exclude test-only sources from the main rust_library, and give dfx-core-vendored_test its own srcs/deps (matching rs/sns/cli's pattern) instead of depending on `crate`. Lead the --test-threads=1 comment with the important reason (tests mutate the file system). Verified via a standalone scratch crate (workspace cargo is blocked by an unrelated private-dep 403): cargo check, cargo test -- --test-threads=1 (3/3 passing), and rustfmt --check all clean. bazel is unavailable in this sandbox, so the BUILD.bazel restructuring is checked by inspection against rs/sns/cli's BUILD.bazel only, not by an actual bazel build/test run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi
This comment was marked as resolved.
This comment was marked as resolved.
Per Daniel Wong's review on #10914: the per-test temp directories weren't actually the whole isolation story. find_project_local_network() read std::env::current_dir(), and shared_local_address() (via get_user_dfx_config_dir()) read the process-global DFX_CONFIG_ROOT static. Tests pointed both at their own temp dirs by mutating this global state (set_current_dir, and swapping DFX_CONFIG_ROOT's Mutex contents), which raced across parallel test threads even though each test's own files were isolated. Both functions had exactly one production call site each, entirely within this crate, so thread the values through as explicit parameters instead: - find_project_local_network now takes start_dir: &Path instead of calling std::env::current_dir() itself. - get_user_dfx_config_dir_with_override is a new sibling of get_user_dfx_config_dir that takes the config root override as a parameter instead of reading DFX_CONFIG_ROOT; the original stays as a thin wrapper for the two existing (unchanged) production callers. - shared_local_address takes an Option<&Path> override, using the ambient DFX_CONFIG_ROOT-backed path when None. - resolve_local_network is now a thin wrapper around resolve_local_network_with(start_dir, config_root_override), which the tests call directly with explicit temp paths. Tests no longer touch any process-global state, so --test-threads=1 comes off the Bazel target. Verified with a standalone scratch crate (real Bazel is unavailable in this sandbox): cargo check/clippy/test all clean, and 30 repeated `cargo test -- --test-threads=8` runs all passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi
…ored network resolution Per Daniel Wong's round-4 review on #10914: - Verified each new NetworkResolutionError variant against real dfx-core (dfx-core 0.4.0, commit 9f41b205390626aa358e8a2a96498bb26e632f13): Config::from_file/from_slice and NetworksConfig::new/from_file always propagate a read or deserialization failure as Err (dfx.json/networks.json are parsed into typed structs, so a wrong-typed "bind" fails the same way). Added ReadProjectDfxJsonFailed and ReadSharedNetworksJsonFailed so a read failure on an existing (but unreadable) file is a real Err instead of being swept under the rug -- previously it was treated as "keep walking up to the parent directory looking for another dfx.json", which is invented behavior that has no dfx-core equivalent and is worse than the original pre-refactor behavior (which just returned None immediately). networks.json's "doesn't exist" vs. "exists but unreadable" cases are now distinguished the same way dfx-core's NetworksConfig::new does. - Simplified two `match`-based error-mapping blocks to `.map_err(...)?`. - Comment cleanup: added "Read it.", "Parse dfx.json.", and "Get network.local out of dfx.json..." comments; dropped a now-redundant "Look up bind ..." comment and reworded its neighbor; dropped an unnecessary second sentence from shared_local_address's doc comment. - network_tests.rs: replaced the hand-rolled unique_temp_dir helper with tempfile::TempDir (already a workspace dependency, and already used elsewhere in rs/sns) for automatic cleanup, and folded the last test's intermediate `result` binding into a single, full-struct assert_eq. - BUILD.bazel: adopted the DEPENDENCIES/DEV_DEPENDENCIES pattern (still in active use for rs/sns et al., see rs/nervous_system/feature_test.md) to de-duplicate deps between the library and test targets, and added tempfile to DEV_DEPENDENCIES. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi
daniel-wong-dfinity-org-twin
dismissed
github-actions[bot]’s stale review
July 28, 2026 11:05
Does not affect canister behavior.
…DFX_CONFIG_ROOT ambiently - resolve_local_network now propagates a std::env::current_dir() failure as Err(DetermineCurrentWorkingDirFailed), matching dfx-core's Config::from_current_dir instead of silently treating it as "no project root found". resolve_local_network_with's start_dir is now &Path, not Option<&Path>. - resolve_local_network now reads the DFX_CONFIG_ROOT override itself and passes it explicitly; shared_local_address no longer falls back to the ambient get_user_dfx_config_dir() when None, so None unambiguously means "DFX_CONFIG_ROOT is not set" rather than also meaning "check ambient state here". - Fix networks.local typo in a code comment. - Add a doc comment on get_user_dfx_config_dir stating its common-case return value (~/.config/dfx). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi
…olution - directories.rs: delete vacuous "Returns the user's dfx config directory." sentence; keep the non-vacuous "usually ~/.config/dfx" explanation. - network.rs: rename start_dir -> working_directory and config_root_override -> dfx_config_root throughout; reflow the resolve_local_network_with doc per suggestion; trim the find_project_local_network doc to end after "the user's project."; drop the ", rather than ... value directly." aside from shared_local_address's doc; replace every "plays the role of" phrasing with a direct statement of what value is passed; attach .as_deref().map(Path::new) to its own statement instead of the call site. - Restructure NetworkResolutionError's JSON-loading variants to actually mirror dfx-core's real layering instead of inventing flat ones: add error/load_dfx_config.rs (LoadDfxConfigError, copied name+shape from dfx-core) and error/load_networks_config.rs (LoadNetworksConfigError, ditto), and deserialize dfx.json/networks.json into small typed structs (mirroring ConfigNetwork::ConfigLocalProvider) via the crate's existing load_json_file/StructuredFileError helper, so a non-string "bind" now fails the same typed deserialization dfx-core would fail, instead of needing a bespoke InvalidLocalNetworkBind variant. - Add regression tests for the non-string "bind" error path on both the project and shared JSON files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi
…ndored-local-network # Conflicts: # Cargo.lock
ReadWebserverPortFailed/ParsePortValueFailed were hand-rolling a
(PathBuf, io::Error) pair and calling raw std::fs::read_to_string,
instead of reusing this crate's own ReadToStringError (error/fs.rs)
and crate::fs::read_to_string helper -- both of which already mirror
dfx-core's fs::read_to_string/ReadToStringError. Real dfx-core's
NetworkConfigError::ReadWebserverPortFailed wraps ReadToStringError
(carrying the path itself) rather than a bare io::Error, and
ParsePortValueFailed boxes its PathBuf/ParseIntError fields with the
message "Failed to parse contents of {0} as a port value"
(error/network_config.rs). Copied both exactly, and switched
get_running_webserver_address to use crate::fs::read_to_string.
Also reflowed the resolve_local_network_with doc comment, which had
an unwrapped 92-character line left over from applying a review
suggestion verbatim without reflowing it as the suggestion itself
asked.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi
…g em dashes Per Daniel Wong's review on #10914: - resolve_local_network: merge the two `let dfx_config_root` statements into one. Chaining `.as_deref().map(Path::new)` directly onto the `DFX_CONFIG_ROOT.lock().unwrap().clone()` temporary in a single `let` doesn't compile (E0716: temporary value dropped while borrowed, since the cloned `Option<OsString>` only lives to the end of that statement but the derived `Option<&Path>` needs to survive into the next line). Fixed by binding an owned `Option<PathBuf>` instead and deferring `.as_deref()` to the call site, which is one statement and compiles. - find_project_local_network doc comment: replace the em-dash aside about a non-string "bind" value with its own declarative sentence. - load_shared_networks_config: same em-dash-to-sentence fix in the networks.json-not-found comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi
daniel-wong-dfinity-org-twin
approved these changes
Aug 5, 2026
…ndored-local-network
daniel-wong-dfinity-org-twin
pushed a commit
to dfinity/snsdemo
that referenced
this pull request
Aug 10, 2026
…around (#616) ## What 1. **Bump `DFX_IC_COMMIT`** in `bin/versions.bash` from `42413a1c7dbabc98ae3abc6124a9b96d3c0985fe` to `1e1a813088f35d1024f9591a0a55617d37f0fb9e` (current tip of `dfinity/ic`'s `master`, at the time this PR was opened, with a green "Kickoff" CI run). This commit includes dfinity/ic#10914, merged as `aeb799e3dd3b5e0cecb628f2070bcb2aa7bc8400`. 2. **Revert #614** (`git revert 1142856`, clean single-commit revert): restores `bin/dfx-sns-wasm-upload`'s `sns add-sns-wasm-for-tests --network "$DFX_NETWORK"` call and removes the `DFX_NNS_URL`/`dfx-network-provider` workaround that #614 added. ## Why dfinity/ic#10914 fixes local-network resolution in the vendored `dfx-core` subset (`rs/sns/dfx-core-vendored`) used by the `sns` CLI: it now correctly falls back to the *shared* `~/.config/dfx/networks.json` (`127.0.0.1:8080`, this repo's convention) when the nearest `dfx.json` doesn't declare its own `networks.local`, instead of hardcoding the dfx project default `127.0.0.1:8000`. That hardcoded-default bug is exactly what #614 worked around, by resolving the network name to a literal URL via `dfx-network-provider` before calling `sns add-sns-wasm-for-tests`. Now that the underlying bug is fixed upstream and this PR's `DFX_IC_COMMIT` bump picks it up, the workaround is no longer needed — passing the network name `local` directly works again, matching how the rest of the `sns` CLI callsites in this repo behave. ## Verification - `git merge-base --is-ancestor aeb799e3dd3b5e0cecb628f2070bcb2aa7bc8400 1e1a813088f35d1024f9591a0a55617d37f0fb9e` confirms the pinned commit includes the dfx-core-vendored fix. - `shellcheck -e SC1090 -e SC2119 -e SC1091 -e SC2121 -e SC2155 -e SC2094 -e SC2015 bin/versions.bash bin/dfx-sns-wasm-upload` (this repo's own `.github/workflows/checks.yml` flags) passes clean. - Full validation relies on this repo's CI, which exercises a real `dfx start`/replica against the pinned IC commit — not reproducible in this environment, hence opening as a draft PR pending CI. Fixes/relates to dfinity/ic#10914, reverts #614. --- Requested by: Daniel Wong 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi --- _Generated by [Claude Code](https://claude.ai/code/session_011Zr39tUEEpEqc45jdQ5HDi)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What broke
#10640 vendored a subset of
dfx-coreintors/sns/dfx-core-vendored, replacingic-sns-cli's externaldfx-coredependency. This broke local-network resolution for downstream consumers, notablydfinity/snsdemo's CI (dfinity/snsdemo#611, dfinity/snsdemo#612), which now fails with:snsdemo's shared local network is configured (via
~/.config/dfx/networks.json) to bind on127.0.0.1:8080, not the default127.0.0.1:8000/127.0.0.1:4943, and snsdemo's projectdfx.jsondoes not declare its ownnetworks.localentry.Why
resolve_local_network()inrs/sns/dfx-core-vendored/src/network.rsdecided "project-scoped local network" vs. "shared local network" purely by whether anydfx.jsonexisted above the working directory (viafind_project_root()):This doesn't check whether that
dfx.jsonactually declares its ownnetworks.localentry. A project like snsdemo's, whosedfx.jsonhas nonetworkskey at all, was wrongly routed into the "project-scoped" branch and given the hardcoded127.0.0.1:8000default — instead of falling back to the shared network and reading its actually-configuredbindaddress.This mirrors a gap the PR's own description called out: the shared-network config reading present in the real
dfx-core(create_shared_network_descriptor, which readsnetworks.json'slocalentry and only falls back to a hardcoded default when that entry is itself absent) was not carried over to the vendored subset. Realdfx-coreonly takes the project-config branch for a network the project'sdfx.jsonactually declares (create_project_network_descriptorreturnsNone— not an error — when the network isn't present, letting resolution fall through to the shared config).What this PR changes
In
rs/sns/dfx-core-vendored/src/network.rs:find_project_root()(found adfx.json, unconditionally treated as project-scoped) withfind_project_local_network(), which returns the project root and its configuredlocalbind address only when the nearestdfx.jsonactually has anetworks.localentry. Otherwise it returnsNone, so resolution falls back to the shared network, matching dfx'screate_project_network_descriptorsemantics.shared_local_address(), which reads the actual configuredbindfrom the shared~/.config/dfx/networks.json(viaget_user_dfx_config_dir()), falling back to the existing127.0.0.1:4943default only when that file doesn't exist or has nolocalentry. Note the sharednetworks.jsonhas no top-levelnetworkskey (it is the network map), unlike a project'sdfx.json.resolve_local_network()to use these instead of hardcodingDEFAULT_SHARED_LOCAL_ADDRESSfor every dfx-project-adjacent-but-not-declaring case.dfx.jsonwith nonetworkskey falling back to the shared network's configured bind (the regression case); a projectdfx.jsonthat does declare its ownlocalnetwork taking precedence; and the shared-network default applying when neither config declareslocal.rust_testtarget toBUILD.bazelfor the new tests (serialized via--test-threads=1, since they mutate the process's working directory and the shared-config-directory override).No change to the external dependency surface — this stays a minimal, in-place bugfix to the vendored subset rather than reintroducing the full
dfx-coredependency.