fix: keep a binding's stamped id when this run cannot resolve it

CHANGELOG.md

@@ -15,6 +15,7 @@ air-gapped signing
 * 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 <id>` 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.
+* fix: a deploy that targets only some of a project's canisters no longer unwires them from the ones it did not target. `PUBLIC_CANISTER_ID:<name>` variables are computed from the ids in the environment's store and written as the canister's whole variable list, so a run that could not see a referenced canister's id — it lies outside the run's scope, or was deployed from another project root, which is how a workspace member deployed on its own sees its dependencies — silently dropped the variable a full deploy had stamped, and the canister read an empty value from then on. Such a variable now keeps the value it has, and the run reports which canister it could not resolve.
 
 ## Experimental
 

crates/icp-cli/tests/dependency_tests.rs

@@ -2,7 +2,11 @@ use indoc::formatdoc;
 use predicates::{prelude::PredicateBooleanExt, str::contains};
 
 use crate::common::{ENVIRONMENT_RANDOM_PORT, NETWORK_RANDOM_PORT, TestContext, clients};
-use icp::{fs::write_string, prelude::*};
+use icp::{
+    fs::{read_to_string, write_string},
+    prelude::*,
+};
+use std::collections::BTreeMap;
 
 mod common;
 
@@ -524,3 +528,150 @@ async fn deploy_with_shared_dependency_dedups_to_one_instance() {
             );
     }
 }
+
+/// The canister id store the CLI keeps for `env` under `project_dir`.
+fn ids_store_path(project_dir: &Path, env: &str) -> PathBuf {
+    for sub in ["cache", "data"] {
+        let p = project_dir.join(format!(".icp/{sub}/mappings/{env}.ids.json"));
+        if p.exists() {
+            return p;
+        }
+    }
+    panic!("no canister id store for environment '{env}' under {project_dir}/.icp");
+}
+
+/// A binding whose target has no id in the store must not unwire the canister.
+/// A deploy scoped to one canister cannot recompute an id it cannot see — the
+/// dependency was deployed from another project root, or lies outside this
+/// run's scope — so it keeps the id already stamped and says why, instead of
+/// replacing the canister's variables with a set that silently omits it.
+#[tokio::test]
+async fn scoped_deploy_keeps_a_dependency_id_it_cannot_resolve() {
+    let ctx = TestContext::new();
+    let project_dir = ctx.create_project_dir("icp");
+    let wasm = ctx.make_asset("example_icp_mo.wasm");
+
+    let dep_dir = project_dir.join("vendor/openemail");
+    std::fs::create_dir_all(&dep_dir).expect("failed to create dependency dir");
+    let dep_manifest = formatdoc! {r#"
+        canisters:
+          - name: backend
+            build:
+              steps:
+                - type: script
+                  command: cp '{wasm}' "$ICP_WASM_OUTPUT_PATH"
+
+        environments:
+          - name: random-environment
+    "#};
+    write_string(&dep_dir.join("icp.yaml"), &dep_manifest)
+        .expect("failed to write dependency manifest");
+
+    let pm = formatdoc! {r#"
+        canisters:
+          - name: app
+            build:
+              steps:
+                - type: script
+                  command: cp '{wasm}' "$ICP_WASM_OUTPUT_PATH"
+
+        dependencies:
+          - name: openemail
+            path: ./vendor/openemail
+
+        {NETWORK_RANDOM_PORT}
+        {ENVIRONMENT_RANDOM_PORT}
+    "#};
+    write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest");
+
+    let _g = ctx.start_network_in(&project_dir, "random-network").await;
+    ctx.ping_until_healthy(&project_dir, "random-network");
+
+    clients::icp(&ctx, &project_dir, Some("random-environment".to_string()))
+        .mint_cycles(200 * TRILLION);
+
+    // A full workspace deploy stamps the real dependency id.
+    ctx.icp()
+        .current_dir(&project_dir)
+        .args(["deploy", "--environment", "random-environment"])
+        .assert()
+        .success();
+
+    let assert = ctx
+        .icp()
+        .current_dir(&project_dir)
+        .args([
+            "canister",
+            "status",
+            "--environment",
+            "random-environment",
+            "vendor/openemail:backend",
+            "--id-only",
+        ])
+        .assert()
+        .success();
+    let openemail_id = String::from_utf8(assert.get_output().stdout.clone())
+        .expect("canister id should be valid utf-8")
+        .trim()
+        .to_string();
+    assert!(
+        !openemail_id.is_empty(),
+        "expected an openemail canister id"
+    );
+
+    let stamped =
+        || contains("PUBLIC_CANISTER_ID:openemail:backend").and(contains(openemail_id.clone()));
+    ctx.icp()
+        .current_dir(&project_dir)
+        .args([
+            "canister",
+            "settings",
+            "show",
+            "app",
+            "--environment",
+            "random-environment",
+        ])
+        .assert()
+        .success()
+        .stdout(stamped());
+
+    // Drop the dependency from the id store this run reads — the state a service
+    // deployed from its own project root, or a fresh checkout, is in.
+    let store = ids_store_path(&project_dir, "random-environment");
+    let mut ids: BTreeMap<String, String> =
+        serde_json::from_str(&read_to_string(&store).expect("failed to read the id store"))
+            .expect("the id store should map canister names to principals");
+    ids.remove("vendor/openemail:backend")
+        .expect("the dependency should be in the store");
+    write_string(
+        &store,
+        &serde_json::to_string(&ids).expect("failed to serialize the id store"),
+    )
+    .expect("failed to write the id store");
+
+    // Re-deploying just the app cannot recompute the id, and says so...
+    ctx.icp()
+        .current_dir(&project_dir)
+        .args(["deploy", "app", "--environment", "random-environment"])
+        .assert()
+        .success()
+        .stderr(
+            contains("vendor/openemail:backend")
+                .and(contains("PUBLIC_CANISTER_ID:openemail:backend")),
+        );
+
+    // ...so it keeps the id already stamped, rather than dropping the variable.
+    ctx.icp()
+        .current_dir(&project_dir)
+        .args([
+            "canister",
+            "settings",
+            "show",
+            "app",
+            "--environment",
+            "random-environment",
+        ])
+        .assert()
+        .success()
+        .stdout(stamped());
+}

crates/icp/src/operations/binding_env_vars.rs

@@ -3,12 +3,14 @@ use std::collections::{BTreeMap, HashSet};
 use crate::Canister;
 use futures::{StreamExt, stream::FuturesOrdered};
 use ic_agent::{Agent, export::Principal};
-use ic_management_canister_types::{CanisterSettings, EnvironmentVariable, UpdateSettingsArgs};
+use ic_management_canister_types::{
+    CanisterIdRecord, CanisterSettings, EnvironmentVariable, UpdateSettingsArgs,
+};
 use icp_events::TaskOutcome;
 
 use crate::operations::task::{Reporter, Task};
-use snafu::Snafu;
-use tracing::error;
+use snafu::{ResultExt, Snafu};
+use tracing::{error, warn};
 
 use super::proxy::UpdateOrProxyError;
 use super::proxy_management;
@@ -21,6 +23,12 @@ pub enum BindingEnvVarsOperationError {
         canister_names: Vec<String>,
     },
 
+    #[snafu(display("failed to fetch current canister settings for canister {canister}"))]
+    FetchCurrentSettings {
+        source: UpdateOrProxyError,
+        canister: Principal,
+    },
+
     #[snafu(transparent)]
     UpdateOrProxy { source: UpdateOrProxyError },
 }
@@ -31,19 +39,65 @@ pub struct SetBindingEnvVarsManyError {
     names: Vec<String>,
 }
 
+/// The environment-variable namespace the project's bindings are stamped into.
+const BINDING_PREFIX: &str = "PUBLIC_CANISTER_ID:";
+
+/// Write a canister's environment variables: the ones its manifest declares,
+/// with the ids it is wired to stamped over them.
+///
+/// `unresolved` names the bindings this run could not compute, as
+/// `(variable, referenced canister)`. Their current values are carried over,
+/// because the update below replaces the canister's whole variable list, and an
+/// unresolved binding means "no id in *this* store" — the dependency was
+/// deployed from another project root, or lies outside this command's scope —
+/// not "this canister is not wired to it". Without that, a deploy scoped to one
+/// service would delete the id a full workspace deploy had stamped, leaving the
+/// canister to read an empty value with nothing said anywhere.
 pub async fn set_env_vars_for_canister(
     agent: &Agent,
     proxy: Option<Principal>,
     canister_id: &Principal,
     canister_info: &Canister,
     binding_vars: &[(String, String)],
+    unresolved: &[(String, String)],
 ) -> Result<(), BindingEnvVarsOperationError> {
     let mut environment_variables = canister_info
         .settings
         .environment_variables
         .to_owned()
         .unwrap_or_default();
 
+    // Only an unresolved binding needs the canister's current state, so the
+    // common path still writes without reading first.
+    if !unresolved.is_empty() {
+        let status = proxy_management::canister_status(
+            agent,
+            proxy,
+            CanisterIdRecord {
+                canister_id: *canister_id,
+            },
+        )
+        .await
+        .context(FetchCurrentSettingsSnafu {
+            canister: *canister_id,
+        })?;
+
+        for (variable, _) in unresolved {
+            let Some(current) = status
+                .settings
+                .environment_variables
+                .iter()
+                .find(|v| &v.name == variable)
+            else {
+                continue;
+            };
+            // A value the manifest declares still outranks a stamped one.
+            environment_variables
+                .entry(variable.to_owned())
+                .or_insert_with(|| current.value.clone());
+        }
+    }
+
     // inject the ids of the other canisters
     for (k, v) in binding_vars.iter() {
         environment_variables.insert(k.to_string(), v.to_string());
@@ -120,22 +174,35 @@ pub async fn set_binding_env_vars_many(
         // their aliases), resolved to the ids that exist in this environment.
         // A project without dependencies wires every canister to every sibling,
         // reproducing the previous flat behavior.
-        let binding_vars: Vec<(String, String)> = info
-            .bindings
-            .iter()
-            .filter_map(|(env_name, referenced_key)| {
-                canister_list.get(referenced_key).map(|principal| {
-                    (
-                        format!("PUBLIC_CANISTER_ID:{env_name}"),
-                        principal.to_text(),
-                    )
-                })
-            })
-            .collect();
+        //
+        // A binding whose target has no id in this environment is kept aside
+        // rather than dropped: the canister may already carry the value, and
+        // silently writing it away is how a scoped deploy used to unwire a
+        // canister from its dependency.
+        let mut binding_vars: Vec<(String, String)> = Vec::new();
+        let mut unresolved: Vec<(String, String)> = Vec::new();
+        for (env_name, referenced_key) in &info.bindings {
+            let variable = format!("{BINDING_PREFIX}{env_name}");
+            match canister_list.get(referenced_key) {
+                Some(principal) => binding_vars.push((variable, principal.to_text())),
+                None => unresolved.push((variable, referenced_key.to_owned())),
+            }
+        }
 
         let agent = agent.clone();
         futs.push_back(async move {
-            let result = set_env_vars_for_canister(&agent, proxy, &cid, &info, &binding_vars).await;
+            for (variable, referenced_key) in &unresolved {
+                warn!(
+                    "Canister '{}' is wired to '{referenced_key}', which has no id in environment \
+                     '{environment_name}'; leaving '{variable}' as it is. Deploy \
+                     '{referenced_key}' to stamp its id.",
+                    info.name
+                );
+            }
+
+            let result =
+                set_env_vars_for_canister(&agent, proxy, &cid, &info, &binding_vars, &unresolved)
+                    .await;
 
             match &result {
                 Ok(()) => task.finish(TaskOutcome::succeeded()),