fix(sns-cli): resolve shared local network config in vendored dfx-core

[claude]

What broke

#10640 vendored a subset of dfx-core into rs/sns/dfx-core-vendored, replacing ic-sns-cli's external dfx-core dependency. This broke local-network resolution for downstream consumers, notably dfinity/snsdemo's CI (dfinity/snsdemo#611, dfinity/snsdemo#612), which now fails with:

Error: Failed to build agent for network `local` and identity `None`
...
Caused by:
    0: Failed to fetch root key from network ...
    1: error sending request for url (http://127.0.0.1:8000/api/v2/status)

snsdemo's shared local network is configured (via ~/.config/dfx/networks.json) to bind on 127.0.0.1:8080, not the default 127.0.0.1:8000/127.0.0.1:4943, and snsdemo's project dfx.json does not declare its own networks.local entry.

Why

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 (via find_project_root()):

let (data_directory, default_address) = match find_project_root() {
    Some(project_root) => (
        project_root.join(".dfx").join("network").join("local"),
        project_local_address(&project_root),
    ),
    None => (
        get_shared_network_data_directory("local")...,
        DEFAULT_SHARED_LOCAL_ADDRESS.to_string(),
    ),
};

This doesn't check whether that dfx.json actually declares its own networks.local entry. A project like snsdemo's, whose dfx.json has no networks key at all, was wrongly routed into the "project-scoped" branch and given the hardcoded 127.0.0.1:8000 default — instead of falling back to the shared network and reading its actually-configured bind address.

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 reads networks.json's local entry and only falls back to a hardcoded default when that entry is itself absent) was not carried over to the vendored subset. Real dfx-core only takes the project-config branch for a network the project's dfx.json actually declares (create_project_network_descriptor returns None — 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:

• Replaced find_project_root() (found a dfx.json, unconditionally treated as project-scoped) with find_project_local_network(), which returns the project root and its configured local bind address only when the nearest dfx.json actually has a networks.local entry. Otherwise it returns None, so resolution falls back to the shared network, matching dfx's create_project_network_descriptor semantics.
• Added shared_local_address(), which reads the actual configured bind from the shared ~/.config/dfx/networks.json (via get_user_dfx_config_dir()), falling back to the existing 127.0.0.1:4943 default only when that file doesn't exist or has no local entry. Note the shared networks.json has no top-level networks key (it is the network map), unlike a project's dfx.json.
• Updated resolve_local_network() to use these instead of hardcoding DEFAULT_SHARED_LOCAL_ADDRESS for every dfx-project-adjacent-but-not-declaring case.
• Updated the module doc comment to describe the corrected behavior.
• Added unit tests covering: a project dfx.json with no networks key falling back to the shared network's configured bind (the regression case); a project dfx.json that does declare its own local network taking precedence; and the shared-network default applying when neither config declares local.
• Added a rust_test target to BUILD.bazel for 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-core dependency.

Does not affect canister behavior.

[daniel-wong-dfinity-org-twin]

Is all of this copied from dfx-core?

[claude]

Breaking down the whole NetworkResolutionError enum, since that's what's in view here:

• NetworkNotFound, DetermineSharedNetworkDirectoryFailed, ReadWebserverPortFailed, ParsePortValueFailed — copied (name + shape) from NetworkConfigError (dfx-core's error/network_config.rs).
• DetermineCurrentWorkingDirFailed — copied verbatim (name + message) from LoadDfxConfigError::DetermineCurrentWorkingDirFailed (error/load_dfx_config.rs, used by Config::from_current_dir).
• DetermineSharedConfigDirectoryFailed — NOT copied, bespoke. dfx-core wraps this transparently as NetworkConfigError::Config(#[from] ConfigError); we give it its own name since this crate doesn't use #[from]-transparent wrapping here.
• ReadProjectDfxJsonFailed, ParseProjectDfxJsonFailed, InvalidProjectLocalNetworkBind, and their shared-network counterparts (ReadSharedNetworksJsonFailed, ParseSharedNetworksJsonFailed, InvalidSharedLocalNetworkBind) — NOT copied, bespoke. dfx-core has no equivalents at this layer at all: it loads dfx.json/networks.json through separate Config/NetworksConfig model types (errors: LoadDfxConfigError, LoadNetworksConfigError/StructuredFileError) that network/provider.rs never sees directly. This crate folds that read+parse step straight into network.rs, so there's no upstream variant to copy — only the underlying behavior (propagate as Err, don't swallow) is verified against dfx-core, per r3664702254.

Summary: behavior is checked against dfx-core throughout; names are copied where dfx-core has a matching variant at this layer, bespoke where it spreads that failure across model types this crate doesn't replicate.

[claude]

No, it didn't match before this round. Restructured it to actually copy dfx-core's real layered types instead.

Went back to real dfx-core (src/dfx-core/src/network/provider.rs, config/model/dfinity.rs, error/{load_dfx_config,load_networks_config,structured_file}.rs) to check each bespoke variant individually:

• ReadProjectDfxJsonFailed / ParseProjectDfxJsonFailed -- these DO have a real upstream equivalent I missed last round: LoadDfxConfigError::ReadFile(#[from] ReadFileError) and LoadDfxConfigError::DeserializeValueFailed(Box<PathBuf>, serde_json::Error), from Config::from_file/from_slice. Added error/load_dfx_config.rs with a trimmed LoadDfxConfigError (name + shape copied), and replaced both variants with NetworkResolutionError::LoadProjectDfxJsonFailed(#[from] LoadDfxConfigError).
• ReadSharedNetworksJsonFailed / ParseSharedNetworksJsonFailed / DetermineSharedConfigDirectoryFailed -- same story: real upstream equivalent is LoadNetworksConfigError::GetConfigPathFailed(ConfigError) / LoadConfigFromFileFailed(StructuredFileError), from NetworksConfig::new/from_file. Added error/load_networks_config.rs with LoadNetworksConfigError (copied name + shape), replaced all three with NetworkResolutionError::LoadSharedNetworksConfigFailed(#[from] LoadNetworksConfigError).
• InvalidProjectLocalNetworkBind / InvalidSharedLocalNetworkBind -- re-verified there's still no dedicated upstream variant for these, but now I see why: dfx-core deserializes dfx.json/networks.json straight into a typed ConfigInterface/NetworksConfigInterface (where bind: Option<String>), so a non-string "bind" fails as part of the ordinary DeserializeValueFailed/DeserializeJsonFileFailed parse -- it's not sufficiently distinct there to warrant its own variant. Our code was hand-walking serde_json::Value, which is what forced a bespoke "invalid bind" variant to exist at all. Fixed by deserializing into small typed structs (ProjectDfxJson, LocalNetworkBind, mirroring ConfigNetwork::ConfigLocalProvider) via the crate's existing load_json_file/StructuredFileError helper (already used elsewhere in this crate, e.g. identity_manager.rs) -- so a non-string bind now fails the same typed deserialization dfx-core would fail, and both bespoke variants are gone entirely.

NetworkNotFound, DetermineCurrentWorkingDirFailed, DetermineSharedNetworkDirectoryFailed, ReadWebserverPortFailed, ParsePortValueFailed are unchanged -- these were already exact copies, reconfirmed.

Net: NetworkResolutionError shrank from 11 variants to 7, and every variant now either is an exact copy or wraps a real upstream error type by name and structure. Added two regression tests for the non-string-bind path (project + shared) since removing the bespoke variants changed which type surfaces that error.

[daniel-wong-dfinity-org-twin]

So, we are now matching dfx-core, right?? Again, this is a yes or no question, so please, do not spew a giant mountain of text on me.

[claude]

Found one real remaining divergence in this enum, now fixed (28492d9):

ReadWebserverPortFailed and ParsePortValueFailed were hand-rolling (PathBuf, io::Error)/(PathBuf, ParseIntError) pairs, and get_running_webserver_address called raw std::fs::read_to_string directly — instead of reusing this crate's own ReadToStringError (error/fs.rs) and crate::fs::read_to_string helper, which already exist in this crate and already mirror dfx-core's fs::read_to_string/ReadToStringError (src/dfx-core/src/fs/mod.rs, error/fs.rs).

Real dfx-core (error/network_config.rs):

ReadWebserverPortFailed(#[source] ReadToStringError),
...
#[error("Failed to parse contents of {0} as a port value")]
ParsePortValueFailed(Box<PathBuf>, #[source] Box<ParseIntError>),

and network/provider.rs::get_running_webserver_bind_address:

let s = crate::fs::read_to_string(&path).map_err(ReadWebserverPortFailed)?;
...
.map_err(|e| ParsePortValueFailed(Box::new(path), Box::new(e)))?;

Changed our ReadWebserverPortFailed to wrap ReadToStringError (dropping the separate PathBuf field, since ReadToStringError already carries the path), boxed both ParsePortValueFailed fields to match, matched its exact message text ("Failed to parse contents of {0} as a port value"), and switched get_running_webserver_address to call crate::fs::read_to_string instead of std::fs::read_to_string.

Everything else in the enum: re-checked against error/network_config.rs, error/load_dfx_config.rs, error/load_networks_config.rs line by line again and it's unchanged from the breakdown in r3706320341/r3711753026 — NetworkNotFound, DetermineSharedNetworkDirectoryFailed are exact copies of NetworkConfigError variants; DetermineCurrentWorkingDirFailed is an exact copy of LoadDfxConfigError::DetermineCurrentWorkingDirFailed; LoadProjectDfxJsonFailed/LoadSharedNetworksConfigFailed wrap the real, separately-vendored LoadDfxConfigError/LoadNetworksConfigError types by name and shape. No remaining bespoke/invented variants at this layer. is_ic, the four DEFAULT_* constants, resolve_network, and get_running_webserver_address's core logic (including the localhost:port → [::1] comment) were also re-diffed against network_descriptor.rs/config/model/dfinity.rs/network/provider.rs and already match exactly.

(Replying here per your note that this is the same underlying issue as the newer comment on line 70 — leaving a short pointer over there.)

[claude]

Yes.

[daniel-wong-dfinity-org-twin]

Returns the user's dfx config directory.

This is vacuous. Never do this type of shit. All this does is take the name of the code, and turns it into proper prose. Nobody benefits from that. Never ever do this horrible garbage.

Usually, the alternative is to define jargon used by the name. In this case, we could try to answer, "WTF even IS a 'user dfx config directory'??" It is not merely any old directory where a "user" might decide to randomly shove "configuration" for dfx. Rather, it is a special place that dfx goes into to look for networks.json (and other shit?). From the typical value, ~/.config/dfx, people would generally be able to figure this out. Therefore, a full explanation is probably not needed in this case. I was just using it as an example.

[claude]

Deleted the vacuous sentence entirely — kept only the "usually ~/.config/dfx, see get_user_dfx_config_dir_with_override" part (directories.rs:29-31), per your clarification that no further explanation is needed here.

[daniel-wong-dfinity-org-twin]

Suggested change

	/// Does the actual work of [`resolve_local_network`], but takes the working
	/// directory and the `DFX_CONFIG_ROOT` override as explicit parameters
	/// instead of reading them from process-global state (`std::env::current_dir`,
	/// and the `DFX_CONFIG_ROOT` mutex). This lets tests supply both directly,
	/// instead of mutating that global state, which would otherwise race across
	/// parallel test threads.
	/// Does the actual work of [`resolve_local_network`], but takes the working
	/// directory and the `DFX_CONFIG_ROOT` environment variable override as explicit parameters
	/// instead of reading them, making this more testable.

(Per standing order, reflow as necessary.)

[claude]

Applied verbatim (network.rs:162-164).

[daniel-wong-dfinity-org-twin]

Reflow!

[claude]

Applied verbatim last round, but that left an unwrapped 92-character line — actually reflowed now (28492d9).

[daniel-wong-dfinity-org-twin]

can you move the dot stuff to the previous statement?

[claude]

Done — .as_deref().map(Path::new) now its own statement right after the let dfx_config_root = DFX_CONFIG_ROOT.lock().unwrap().clone(); line, instead of being inlined into the resolve_local_network_with(...) call (network.rs:154-159).

[daniel-wong-dfinity-org-twin]

No, I mean, can't you just do

let dfx_config_root = one
    .big()
    .long()
    .dot()
    .chain();

not two separate ones?

[claude]

Done — single statement now (network.rs:165). Chaining .as_deref().map(Path::new) directly onto the .clone() temporary in one let doesn't compile (E0716, temporary dropped while borrowed), so instead the single statement produces an owned Option<PathBuf> via .map(PathBuf::from), and .as_deref() moves to the call site: resolve_local_network_with(&working_directory, dfx_config_root.as_deref()).

[daniel-wong-dfinity-org-twin]

Instead of em dash, just explain what happens when "bind" is not a string in a separate sentence.

[claude]

Done — network.rs:213-216, now: "Returns Err if the nearest dfx.json exists, but cannot be read or parsed as JSON. A "bind" value that is present but not a string fails that same parse, so it returns Err too."

[daniel-wong-dfinity-org-twin]

This change is more radical than I had expected. Why did you do it this way instead of something more incremental? I'm not asking you to back out of this radical change. Just asking a question.

[claude]

Because an incremental patch (keeping manual serde_json::Value navigation, just adding a hand-rolled "is this a string" check for bind) would itself have been invented behavior with no real dfx-core equivalent — the exact thing you flagged earlier in this review (r3711415583: "you invented some shit here").

dfx-core doesn't have a dedicated non-string-bind check anywhere: it deserializes dfx.json/networks.json straight into typed ConfigInterface/NetworksConfigInterface structs, where bind: Option<String> is an ordinary typed field, so a wrongly-typed value just fails the normal deserialization dfx-core already does for any other reason (missing field, wrong type, etc.). To actually reproduce that — not just approximate its outward behavior — this code had to deserialize into equivalent typed structs (ProjectDfxJson/LocalNetworkBind), which in turn meant using the real, separately-vendored LoadDfxConfigError/LoadNetworksConfigError types dfx-core produces from that deserialization, instead of the bespoke flat variants (InvalidProjectLocalNetworkBind etc.) added in the previous round.

[daniel-wong-dfinity-org-twin]

Ditto.

[claude]

Done — network.rs:310-311, now: "...which only defaults when the file doesn't exist. If it exists but can't be read or parsed, that's an Err instead..."