From 0e9401ad42b5a31608dc7f5a4b9a76dd82c5833a Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Sun, 30 Aug 2026 19:43:05 -0400 Subject: [PATCH] fix: sync the settings the environment declares, not the base ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `icp canister settings sync` resolved its canister through `Context::get_canister_and_path_for_env`, which returned the record from `Project::canisters` and used the environment only to check membership. Consolidation layers an environment's `settings:` overrides onto a *clone* of that base record, so the base one still carries the pre-override settings: a setting declared only under `environments:` was never synced, and the command reported success having changed nothing. That bites hardest in a workspace, because folding a vendored member's own `environments:` config into the root's same-named environments is how a member keeps its standalone settings — so a service configuring itself per environment had those settings ignored for every canister it owns. `icp deploy` already read the environment's record for its settings phases (`env.get_canister_info`), and only used the base record to build, so the two commands disagreed about the same canister in the same environment. Return the environment's record instead. It is a strict refinement of the base one, and the two other callers consult build and sync steps, which are not environment-overridable. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + .../icp-cli/tests/canister_settings_tests.rs | 111 ++++++++++++++++++ crates/icp/src/context/mod.rs | 16 ++- crates/icp/src/context/tests.rs | 83 +++++++++++++ 4 files changed, 207 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c600bf185..48ee145a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ air-gapped signing * fix: canister settings from the manifest are no longer silently discarded when a canister is created through the legacy management-canister fallback (a CloudEngine subnet with no registered engine operator). That path went through `ic-utils`, which encodes `create_canister`'s argument as a bare `canister_settings` record rather than the `record { settings : opt canister_settings; ... }` the interface spec defines, so the replica read no settings at all and created the canister with defaults. `icp deploy` masked this by syncing settings afterwards; `icp canister create` does not, and left the canister unconfigured. * fix: `icp canister logs` output formats are corrected. `--json` now emits machine-readable JSON and the default emits the human-readable lines (the two were swapped), and `--follow --json` emits newline-delimited JSON, one record per line, streamed as each record arrives. This is breaking for scripts: parsing the default output as JSON now requires `--json`, and consumers of `--follow --json` must read one JSON object per line. * fix: `icp canister status` again falls back on the publicly readable state-tree information when the caller may not read the status. Replicas now reject those calls with `IC0542`, which the fallback did not recognise, so the command failed with `Error looking up canister ` instead of printing the controllers and module hash. `IC0541`, returned on subnets with administrators, is now recognised too, and the fallback no longer depends on whether the rejection arrives certified or uncertified. +* fix: `icp canister settings sync` now applies the settings the selected environment declares, not only the canister's base settings. It resolved the canister from the project's pre-override record, so a `settings:` block under `environments:` — including the one a vendored member declares for its own environments — was ignored: a setting declared only there was never written, and the command reported success having changed nothing. `icp deploy` already read the environment's record, so the two disagreed about the same canister in the same environment. ## Experimental diff --git a/crates/icp-cli/tests/canister_settings_tests.rs b/crates/icp-cli/tests/canister_settings_tests.rs index f3ece0c03..fcdfb2256 100644 --- a/crates/icp-cli/tests/canister_settings_tests.rs +++ b/crates/icp-cli/tests/canister_settings_tests.rs @@ -1661,3 +1661,114 @@ async fn canister_settings_show_not_a_controller() { .success() .stdout(contains(principal_alice.as_str())); } + +/// `settings sync` applies the settings the *environment* declares, not the +/// canister's base settings. Consolidation layers an environment's `settings:` +/// override onto a clone of the base record, so reading the base record syncs +/// pre-override settings — and for a variable declared only in the override, +/// syncs nothing at all. +#[tokio::test] +async fn canister_settings_sync_applies_environment_override() { + let ctx = TestContext::new(); + + // Setup project + let project_dir = ctx.create_project_dir("icp"); + + // Use vendored WASM + let wasm = ctx.make_asset("example_icp_mo.wasm"); + + // `API_KEY` is declared only by the environment override — the canister's + // own settings block never mentions it. + let pm = formatdoc! {r#" + canisters: + - name: my-canister + build: + steps: + - type: script + command: cp '{wasm}' "$ICP_WASM_OUTPUT_PATH" + + {NETWORK_RANDOM_PORT} + + environments: + - name: random-environment + network: random-network + settings: + my-canister: + environment_variables: + API_KEY: from-the-environment + "#}; + + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + + // Start network + let _g = ctx.start_network_in(&project_dir, "random-network").await; + ctx.ping_until_healthy(&project_dir, "random-network"); + + // Deploy project + clients::icp(&ctx, &project_dir, Some("random-environment".to_string())) + .mint_cycles(200 * TRILLION); + + ctx.icp() + .current_dir(&project_dir) + .args(["deploy", "--environment", "random-environment"]) + .assert() + .success(); + + // Drop the variable on-canister, so only `settings sync` can put it back. + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "settings", + "update", + "my-canister", + "--environment", + "random-environment", + "--remove-environment-variable", + "API_KEY", + ]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "settings", + "show", + "my-canister", + "--environment", + "random-environment", + ]) + .assert() + .success() + .stdout(contains("API_KEY").not()); + + // Sync must restore it from the environment override. + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "settings", + "sync", + "my-canister", + "--environment", + "random-environment", + ]) + .assert() + .success(); + + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "settings", + "show", + "my-canister", + "--environment", + "random-environment", + ]) + .assert() + .success() + .stdout(contains("API_KEY: from-the-environment")); +} diff --git a/crates/icp/src/context/mod.rs b/crates/icp/src/context/mod.rs index 3ae5ea72a..71f9203d7 100644 --- a/crates/icp/src/context/mod.rs +++ b/crates/icp/src/context/mod.rs @@ -249,27 +249,35 @@ impl Context { } } + /// The canister as it is configured *in this environment*. + /// + /// The record comes from the environment's own canister map, not from + /// [`crate::Project::canisters`]: consolidation layers the environment's `settings:` + /// overrides — the root's, and each vendored member's for its own + /// environments — onto a clone of the base record, so the base one still + /// carries the pre-override settings. Reading it here would silently apply + /// the wrong settings for every canister an environment overrides. pub async fn get_canister_and_path_for_env( &self, canister_name: &str, environment: &EnvironmentSelection, ) -> Result<(PathBuf, Canister), GetEnvCanisterError> { let p = self.project.load().await?; - let Some((path, canister)) = p.get_canister(canister_name) else { + if p.get_canister(canister_name).is_none() { return CanisterNotFoundInProjectSnafu { canister_name: canister_name.to_owned(), } .fail(); - }; + } let env = self.get_environment(environment).await?; - if !env.contains_canister(canister_name) { + let Some((path, canister)) = env.canisters.get(canister_name) else { return CanisterNotInEnvSnafu { canister_name: canister_name.to_owned(), environment_name: environment.name().to_owned(), } .fail(); - } + }; Ok((path.clone(), canister.clone())) } diff --git a/crates/icp/src/context/tests.rs b/crates/icp/src/context/tests.rs index 4c25ea726..eb44efd4a 100644 --- a/crates/icp/src/context/tests.rs +++ b/crates/icp/src/context/tests.rs @@ -953,3 +953,86 @@ async fn test_get_agent_explicit_environment_inside_project() { // Should use the network from the "test" environment (which is "staging") assert_eq!(agent.read_root_key(), staging_root_key); } + +/// The canister must come back as the *environment* configures it. Consolidation +/// layers an environment's `settings:` overrides — the root's, and each vendored +/// member's for its own environments — onto a clone of the base record, so the +/// base one still holds the pre-override settings. `icp canister settings sync` +/// writes whatever this hands back. +#[tokio::test] +async fn get_canister_for_env_returns_the_environments_settings_not_the_base() { + let mut project = MockProjectLoader::minimal().project; + + // The base record holds the pre-override settings... + project + .canisters + .get_mut("backend") + .unwrap() + .1 + .settings + .environment_variables = None; + + // ...while the environment holds the override declared for it. + let overridden = HashMap::from([("API_KEY".to_string(), "from-the-environment".to_string())]); + project + .environments + .get_mut("default") + .unwrap() + .canisters + .get_mut("backend") + .unwrap() + .1 + .settings + .environment_variables = Some(overridden.clone()); + + let ctx = Context { + project: Arc::new(MockProjectLoader::new(project)), + ..Context::mocked() + }; + + let (_, canister) = ctx + .get_canister_and_path_for_env( + "backend", + &EnvironmentSelection::Named("default".to_string()), + ) + .await + .expect("backend is declared in the default environment"); + + assert_eq!(canister.settings.environment_variables, Some(overridden)); +} + +/// A canister the project declares but the selected environment does not is +/// still reported as missing from that environment, not silently served from +/// the project's base map. +#[tokio::test] +async fn get_canister_for_env_rejects_a_canister_the_environment_omits() { + let mut project = MockProjectLoader::minimal().project; + project + .environments + .get_mut("default") + .unwrap() + .canisters + .shift_remove("backend"); + + let ctx = Context { + project: Arc::new(MockProjectLoader::new(project)), + ..Context::mocked() + }; + + let error = ctx + .get_canister_and_path_for_env( + "backend", + &EnvironmentSelection::Named("default".to_string()), + ) + .await + .expect_err("backend is not in the default environment"); + + assert!( + matches!( + &error, + GetEnvCanisterError::CanisterNotInEnv { canister_name, environment_name } + if canister_name == "backend" && environment_name == "default" + ), + "unexpected error: {error}" + ); +}