Skip to content

fix: sync the settings the environment declares, not the base ones - #748

Open
raymondk wants to merge 1 commit into
mainfrom
rk/settings-sync
Open

fix: sync the settings the environment declares, not the base ones#748
raymondk wants to merge 1 commit into
mainfrom
rk/settings-sync

Conversation

@raymondk

Copy link
Copy Markdown
Collaborator

Problem

icp canister settings sync resolves its canister through Context::get_canister_and_path_for_env. Despite the name, that accessor returned the record from Project::canisters — the base, pre-override map — and used the environment only for a membership check:

let Some((path, canister)) = p.get_canister(canister_name) else { ... };  // base map
let env = self.get_environment(environment).await?;
if !env.contains_canister(canister_name) { ... }                         // membership only
Ok((path.clone(), canister.clone()))                                     // base record

Consolidation layers an environment's settings: overrides onto a clone of the base record (build_environment_canisters), so the base one still carries the pre-override settings. A setting declared only under environments: was therefore never synced — and because sync_settings treats None as "leave alone", the command exited 0 having changed nothing, with no warning.

This bites hardest in a workspace with dependencies:. Folding a vendored member's own environments: config into the root's same-named environments is how a member keeps the settings it would have standalone, so a service that configures itself per environment had those settings silently ignored for every canister it owns.

icp deploy did not have the bug: it uses the base record only to build, and switches to the environment's record for its settings phases (env.get_canister_info). So the two commands disagreed about the same canister in the same environment.

Fix

Return the environment's record. It is a strict refinement of the base one, and the two other callers (commands/build.rs, commands/sync.rs) consult build and sync steps, which are not environment-overridable.

Tests

  • canister_settings_sync_applies_environment_override — an integration test that deploys, removes the variable on-canister, and requires settings sync to restore it from the environment override. Without the fix it fails with the variable still absent.
  • Two unit tests on the accessor: it returns the environment's settings over the base ones, and still reports a canister the environment omits as missing rather than serving it from the base map.

cargo test -p icp --lib, --test canister_settings_tests, --test dependency_tests and --test deploy_tests pass, plus cargo fmt and cargo clippy --all-targets. The one failure locally is deploy_cloud_engine::docker::test, which needs a Docker daemon I don't have running.

Not covered here

settings sync still never stamps PUBLIC_CANISTER_ID:*. Those are computed as bindings and written by set_binding_env_vars_many, which only deploy calls — so sync can neither add a missing dependency id nor correct a stale one. That is a separate gap.

🤖 Generated with Claude Code

`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) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings August 30, 2026 23:43
@raymondk
raymondk requested a review from a team as a code owner August 30, 2026 23:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes environment-specific settings synchronization by returning the selected environment’s resolved canister configuration.

Changes:

  • Resolves canisters from the environment map while preserving existing errors.
  • Adds unit and integration regression tests.
  • Documents the fix in the changelog.

Reviewed changes

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

File Description
CHANGELOG.md Records the settings-sync fix.
crates/icp/src/context/mod.rs Returns environment-resolved canister settings.
crates/icp/src/context/tests.rs Tests overrides and omitted canisters.
crates/icp-cli/tests/canister_settings_tests.rs Verifies sync restores an environment variable.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants