From c44babef7b371ab88446b1dae9a52cd3089c0ed1 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Thu, 10 Sep 2026 07:57:08 -0700 Subject: [PATCH 1/5] ci: check that icp-project's core reaches no host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The point of the split was that `icp-project` should end up runnable inside a canister, and nothing was checking it. This adds the job that does — `cargo clippy -p icp-project --no-default-features --target wasm32-unknown-unknown`, on a target where reaching the host does not compile — and fixes everything it found. It has to be its own invocation. A `--workspace` build unifies the `host` feature back on because `icp-cli` enables it, so no whole-workspace command can express this. Which also means `host` has to be strictly additive: it may add implementations, never change what the rest of the crate does, or the job proves nothing about the binary we ship. That is now written down where the feature is declared. Two of the findings were capabilities the core genuinely needs and was taking directly, so they became seams: - `files::FileSystem::scratch_dir` returns a `ScratchDir` handle. `build_many` was calling `camino_tempfile::tempdir()`; a build pipeline does need somewhere one step can leave a wasm for the next step, and then the operation, to read back — so it is something to ask for. - `random::Random`, for `create.rs` picking a subnet when several are available. Its method is `index_below(count)` rather than a byte buffer, because a choice is what every caller wants and turning bytes into an unbiased index is exactly what each one would get subtly wrong. A canister reaches this through `raw_rand`, which is why it is async. The rest is honest gating. `operations::bundle` writes a `.tar.gz` to a disk and puts a plugin's declared directory in as a tree walked for symlinks, which no seam over `FileSystem` reproduces faithfully; `canister::script` spawns a subprocess, and `Builder`/`HostScripts` need it; `stop_signal` off-host waits forever, since there is no process to interrupt and the caller's `select!` arm simply never fires; and the `ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS` read is a host override of a default that stands on its own. One finding was outside this crate. `tokio`'s workspace entry carried `rt-multi-thread`, which no wasm target supports, and feature inheritance only ever adds — so `icp-project` could not opt out of it, and neither could `icp-events`, which this refactor has been claiming is wasm-clean all along. The workspace entry is now the common minimum, and `icp-cli` and `icp-sync-plugin` ask for `rt-multi-thread` where they need it. A second step checks the direction of the dependency itself: no `icp-app` in `icp-project`'s or `icp-sync-plugin`'s manifest, and no `pub use icp_project` facade in `icp-app`. Neither is something a build would catch — a cycle is what cargo would report, and there is nothing to report until someone writes one. Known limitation, left deliberately: `Builder` dispatches both prebuilt steps (which need only `wasm::Fetch` and `files`) and script steps (which need a subprocess), so gating it takes `prebuilt` with it and leaves `canister::build` off-host with a trait and no implementation. Splitting it the way `Syncer` is split — an injected build-script runner — is the fix, and is a design change the gate does not require, so it is follow-up rather than something to smuggle in here. Also dropped `serde_cbor` and `ic-utils`, which nothing in `icp-project` used. Verified: the new job's exact command is clean; 442 unit tests; and 100 integration tests across build (12), create (17), bundle (29), deploy (25) and sync (17) — the suites that cover the scratch directory, the subnet choice and the archive writer. --- .claude/CLAUDE.md | 20 ++++- .github/workflows/checks.yml | 49 +++++++++++++ Cargo.lock | 1 - Cargo.toml | 4 +- crates/icp-app/src/context/init.rs | 1 + crates/icp-cli/Cargo.toml | 2 +- .../icp-cli/src/commands/canister/create.rs | 10 ++- crates/icp-project/Cargo.toml | 29 ++++---- crates/icp-project/src/canister/build/mod.rs | 13 ++++ crates/icp-project/src/canister/mod.rs | 1 + crates/icp-project/src/canister/sync/mod.rs | 5 +- .../icp-project/src/canister/sync/plugin.rs | 8 ++ .../icp-project/src/canister/sync/script.rs | 3 + crates/icp-project/src/host.rs | 4 + crates/icp-project/src/lib.rs | 1 + crates/icp-project/src/operations/bundle.rs | 1 + crates/icp-project/src/operations/create.rs | 15 +++- crates/icp-project/src/operations/deploy.rs | 1 + crates/icp-project/src/operations/mod.rs | 4 + crates/icp-project/src/random.rs | 73 +++++++++++++++++++ crates/icp-project/src/signal.rs | 15 +++- crates/icp-sync-plugin/Cargo.toml | 4 +- 22 files changed, 232 insertions(+), 32 deletions(-) create mode 100644 crates/icp-project/src/random.rs diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 25c8a1bb0..c68913f62 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -12,6 +12,8 @@ cargo test # Run all tests (launcher auto-downloads on cargo test -p icp-cli # Tests for a specific package cargo test --test -- # Specific test cargo fmt && cargo clippy # Run after changes pass tests +# `icp-project`'s core must reach nothing only a host has — see the boundary below +cargo clippy -p icp-project --no-default-features --target wasm32-unknown-unknown ./scripts/generate-cli-docs.sh # Regenerate CLI docs when commands change ./scripts/generate-config-schemas.sh # Regenerate schema when manifest types change ``` @@ -52,14 +54,26 @@ reverse: - `network::Access` — a network's endpoints, root key and friendly domains - `canister::wasm::Fetch` — a wasm module a manifest names by URL - `canister::recipe::Resolve` — a recipe's Handlebars template +- `canister::build::Build` — running one build step - `canister::sync::plugin::Run` — running one sync plugin - `canister::sync::script::ScriptRunner` — running one sync script - `store_id::Access` / `store_artifact::Access` — the project's `.icp` stores +- `random::Random` — a choice that cannot be made by arithmetic - `host::Observe` — what resolution turned up, for telemetry -Host implementations of the first, the last two stores and the script runner -ship in `icp-project` itself behind the default-on `host` feature; the rest -have no implementation there at all. +Some of those have a host implementation in `icp-project` itself, behind the +default-on **`host`** feature: `HostFileSystem`, the two stores, `HostScripts`, +`HostRandom`, and `Builder`. The rest have none there at all. + +`host` must be **strictly additive** — it may add implementations, never change +what the rest of the crate does. CI checks the core with +`cargo clippy -p icp-project --no-default-features --target +wasm32-unknown-unknown`, on a target where reaching the host does not compile; +if `host` changed behaviour rather than adding to it, that check would prove +nothing about the shipped binary. Whole modules that are irreducibly host-side +sit behind it too — `operations::bundle` writes a `.tar.gz` and walks a +directory tree for symlinks, which no seam over `FileSystem` reproduces +faithfully. Because those are implemented across a crate boundary, their error types carry their cause boxed and pass it through with `#[snafu(transparent)]`, which leaves diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 076741383..5a586f4d9 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -84,6 +84,55 @@ jobs: env: RUST_BACKTRACE: 1 + # `icp-project` is meant to end up runnable inside a canister, so its core + # must reach nothing that only a host has. Nothing else expresses this: a + # `--workspace` build unifies the `host` feature back on, because `icp-cli` + # enables it. So this builds that one crate, alone, for a target where + # reaching the host does not compile. + canister-target: + name: canister-target:required + needs: [changes, compile] + if: needs.changes.outputs.src == 'true' + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Setup image (Linux) + run: ./.github/scripts/provision-linux-build.sh + + # rust-cache hashes all installed toolchains; the runner image's `stable` + # drifts as the image updates, which moves the cache key and causes misses. + # Remove it so only the rust-toolchain.toml-pinned version remains. + - name: Remove the runner's bundled Rust toolchain + run: rustup toolchain remove stable 2>/dev/null || true + + - uses: actions-rust-lang/setup-rust-toolchain@150fca883cd4034361b621bd4e6a9d34e5143606 # v1.15.4 + with: + target: wasm32-unknown-unknown + cache-shared-key: ${{ runner.os }}-canister-target + cache-bin: false + + - name: Check icp-project without a host + run: | + cargo clippy -p icp-project --no-default-features \ + --target wasm32-unknown-unknown -- -D warnings + env: + RUST_BACKTRACE: 1 + + # The check above proves the core reaches no host. This proves it reaches + # no *crate above it* either, which no build would catch, because a cycle + # is what cargo would report and there is no cycle to report until + # someone writes one. + - name: Check the dependency direction + run: | + fail() { echo "::error::$1"; exit 1; } + grep -n 'icp-app' crates/icp-project/Cargo.toml crates/icp-sync-plugin/Cargo.toml \ + && fail "icp-project and icp-sync-plugin must not depend on icp-app" + grep -rn 'pub use icp_project' crates/icp-app/src \ + && fail "icp-app must not re-export icp-project: a command reaches for the crate that owns what it needs" + echo "the boundary holds" + format: name: fmt:required needs: [changes, compile] diff --git a/Cargo.lock b/Cargo.lock index 2eb4681e0..b64aac00a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3827,7 +3827,6 @@ dependencies = [ "schemars", "semver", "serde", - "serde_cbor", "serde_json", "serde_yaml", "sha2 0.11.0", diff --git a/Cargo.toml b/Cargo.toml index 903d4b944..6840e3254 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -109,7 +109,9 @@ tempfile = "3" test-tag = "0.1" time = { version = "0.3.47", features = ["formatting", "macros", "parsing"] } tiny-bip39 = "2.0.0" -tokio = { version = "1.45.0", features = ["macros", "rt-multi-thread"] } +# The most any crate here can have in common: `rt-multi-thread` is not +# available on wasm, so the crates that need it ask for it themselves. +tokio = { version = "1.45.0", features = ["macros"] } tracing = "0.1.41" tracing-subscriber = "0.3.20" url = { version = "2.5.4", features = ["serde"] } diff --git a/crates/icp-app/src/context/init.rs b/crates/icp-app/src/context/init.rs index fce33938c..b0dc0bf0d 100644 --- a/crates/icp-app/src/context/init.rs +++ b/crates/icp-app/src/context/init.rs @@ -167,6 +167,7 @@ pub fn initialize( syncer, wasm, network: netaccess.clone(), + random: Arc::new(icp_project::random::HostRandom), observer: telemetry_data.clone(), }, dirs, diff --git a/crates/icp-cli/Cargo.toml b/crates/icp-cli/Cargo.toml index 47215ff6a..319d5e1e3 100644 --- a/crates/icp-cli/Cargo.toml +++ b/crates/icp-cli/Cargo.toml @@ -67,7 +67,7 @@ snafu.workspace = true sysinfo.workspace = true tiny-bip39.workspace = true time.workspace = true -tokio.workspace = true +tokio = { workspace = true, features = ["rt-multi-thread"] } tracing-subscriber.workspace = true tracing.workspace = true url.workspace = true diff --git a/crates/icp-cli/src/commands/canister/create.rs b/crates/icp-cli/src/commands/canister/create.rs index 36d814f37..2280ccd2b 100644 --- a/crates/icp-cli/src/commands/canister/create.rs +++ b/crates/icp-cli/src/commands/canister/create.rs @@ -307,8 +307,13 @@ async fn create_canister(ctx: &Context, args: &CreateArgs) -> Result<(), anyhow: let calls = icp_app::calls::calls(agent.clone(), args.proxy)?; - let create_operation = - CreateOperation::new(calls, args.create_target(), args.funding(), vec![]); + let create_operation = CreateOperation::new( + calls, + ctx.host.random.clone(), + args.create_target(), + args.funding(), + vec![], + ); let canister_settings = args.canister_settings(); @@ -376,6 +381,7 @@ async fn create_project_canister(ctx: &Context, args: &CreateArgs) -> Result<(), let create_operation = CreateOperation::new( calls.clone(), + ctx.host.random.clone(), args.create_target(), args.funding(), ids.values().copied().collect(), diff --git a/crates/icp-project/Cargo.toml b/crates/icp-project/Cargo.toml index ecc8c42ba..a7feded51 100644 --- a/crates/icp-project/Cargo.toml +++ b/crates/icp-project/Cargo.toml @@ -8,16 +8,14 @@ publish.workspace = true [features] default = ["host"] # Implementations of this crate's seams that use the machine it is running on: -# the filesystem and the project-local `.icp` stores. Turned off for a build -# that has to run somewhere without them, such as inside a canister. +# the filesystem, the project-local `.icp` stores, subprocesses, randomness. +# Turned off for a build that has to run somewhere without them, such as inside +# a canister — which CI checks by building this crate for +# `wasm32-unknown-unknown` with default features off. # -# It does not yet cover everything host-shaped here — the step runners still -# spawn subprocesses and drive a wasmtime sandbox unconditionally, `create` -# draws entropy from `rand`, and a bundle's plugin directories go through -# `tar`'s own directory walk. Each is a seam of its own to draw, and every -# dependency is still non-optional besides, so this is the boundary the feature -# claims rather than a canister target. -host = [] +# Strictly additive: it may add implementations, never change what the rest of +# the crate does, or the check proves nothing about the shipped binary. +host = ["dep:camino-tempfile", "dep:rand", "dep:tar", "tokio/io-std", "tokio/process", "tokio/signal"] # Exposes this crate's mocks and fixtures so downstream crates can test against # the same seams. Off in a normal build, so none of it ships. test-util = [] @@ -26,7 +24,7 @@ test-util = [] async-trait = { workspace = true } bigdecimal = { workspace = true } camino = { workspace = true } -camino-tempfile = { workspace = true } +camino-tempfile = { workspace = true, optional = true } candid = { workspace = true } candid_parser = { workspace = true } clap = { workspace = true, optional = true } @@ -48,22 +46,21 @@ num-bigint = { workspace = true } num-integer = { workspace = true } num-traits = { workspace = true } pathdiff = { workspace = true } -rand = { workspace = true } +rand = { workspace = true, optional = true } schemars = { workspace = true } semver = { workspace = true } serde = { workspace = true } -serde_cbor = { workspace = true } serde_json = { workspace = true } serde_yaml = { workspace = true } sha2 = { workspace = true } shellwords = { workspace = true } snafu = { workspace = true } strum = { workspace = true } -tar = { workspace = true } +tar = { workspace = true, optional = true } time = { workspace = true } -# `io-std` is listed for `tokio::io::stdout`/`stderr`; feature unification with other -# crates supplies it anyway, so dropping it would not fail the build. -tokio = { workspace = true, features = ["sync", "macros", "rt", "time", "io-util", "io-std", "process", "signal"] } +# Everything a canister build can have: no `process`, `signal` or `io-std`, +# which the `host` feature adds. +tokio = { workspace = true, features = ["sync", "macros", "rt", "time", "io-util"] } tracing = { workspace = true } url = { workspace = true } wasmparser = { workspace = true } diff --git a/crates/icp-project/src/canister/build/mod.rs b/crates/icp-project/src/canister/build/mod.rs index 7cefb961a..534de549b 100644 --- a/crates/icp-project/src/canister/build/mod.rs +++ b/crates/icp-project/src/canister/build/mod.rs @@ -3,13 +3,18 @@ use async_trait::async_trait; use icp_events::StepReporter; use snafu::prelude::*; +#[cfg(feature = "host")] use std::sync::Arc; +#[cfg(feature = "host")] use crate::canister::wasm; use crate::manifest::canister::BuildStep; use crate::prelude::*; +// Both halves of the one implementation below, which is host-side. +#[cfg(feature = "host")] mod prebuilt; +#[cfg(feature = "host")] mod script; pub struct Params { @@ -20,8 +25,10 @@ pub struct Params { #[derive(Debug, Snafu)] pub enum BuildError { + #[cfg(feature = "host")] #[snafu(transparent)] Script { source: super::script::ScriptError }, + #[cfg(feature = "host")] #[snafu(transparent)] Prebuilt { source: prebuilt::PrebuiltError }, } @@ -38,17 +45,23 @@ pub trait Build: Sync + Send { /// Runs each build step where it has to be run: a script step in a subprocess, /// a pre-built step by asking [`wasm::Fetch`] for the module. +/// +/// Only a host can run the script half, so this whole implementation is +/// host-side; somewhere without subprocesses supplies its own [`Build`]. +#[cfg(feature = "host")] pub struct Builder { wasm: Arc, files: Arc, } +#[cfg(feature = "host")] impl Builder { pub fn new(wasm: Arc, files: Arc) -> Self { Self { wasm, files } } } +#[cfg(feature = "host")] #[async_trait] impl Build for Builder { async fn build( diff --git a/crates/icp-project/src/canister/mod.rs b/crates/icp-project/src/canister/mod.rs index 1c7487ecb..2cfaabf07 100644 --- a/crates/icp-project/src/canister/mod.rs +++ b/crates/icp-project/src/canister/mod.rs @@ -15,6 +15,7 @@ pub mod recipe; pub mod sync; pub mod visibility; +#[cfg(feature = "host")] mod script; pub mod wasm; diff --git a/crates/icp-project/src/canister/sync/mod.rs b/crates/icp-project/src/canister/sync/mod.rs index f5bc22b29..d512969d5 100644 --- a/crates/icp-project/src/canister/sync/mod.rs +++ b/crates/icp-project/src/canister/sync/mod.rs @@ -16,7 +16,9 @@ pub mod declared; pub mod plugin; pub mod script; -use script::{HostScripts, ScriptInvocation, ScriptRunError, ScriptRunner}; +#[cfg(feature = "host")] +use script::HostScripts; +use script::{ScriptInvocation, ScriptRunError, ScriptRunner}; pub struct Params { pub path: PathBuf, @@ -75,6 +77,7 @@ pub struct Syncer { impl Syncer { /// A syncer that runs script steps as host subprocesses. + #[cfg(feature = "host")] pub fn host(wasm: Arc, plugins: Arc) -> Self { Self::new(Arc::new(HostScripts), wasm, plugins) } diff --git a/crates/icp-project/src/canister/sync/plugin.rs b/crates/icp-project/src/canister/sync/plugin.rs index 756194b0f..75edd7138 100644 --- a/crates/icp-project/src/canister/sync/plugin.rs +++ b/crates/icp-project/src/canister/sync/plugin.rs @@ -187,6 +187,7 @@ pub enum PluginError { /// [`PLUGIN_COMPUTE_LIMIT_ENV`] override. Fails loudly on a malformed value so /// a typo doesn't silently fall back to the default and leave the caller /// wondering why their raised limit had no effect. +#[cfg(feature = "host")] fn resolve_compute_limit_secs() -> Result { match std::env::var(PLUGIN_COMPUTE_LIMIT_ENV) { Ok(value) => parse_compute_limit(&value), @@ -201,6 +202,13 @@ fn resolve_compute_limit_secs() -> Result { } } +/// The default, where there are no environment variables to override it with. +#[cfg(not(feature = "host"))] +fn resolve_compute_limit_secs() -> Result { + Ok(DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS) +} + +#[cfg(feature = "host")] fn parse_compute_limit(value: &str) -> Result { match value.trim().parse::() { Ok(secs) if secs >= 1 => Ok(secs), diff --git a/crates/icp-project/src/canister/sync/script.rs b/crates/icp-project/src/canister/sync/script.rs index 82732d6a0..3a77fca21 100644 --- a/crates/icp-project/src/canister/sync/script.rs +++ b/crates/icp-project/src/canister/sync/script.rs @@ -21,6 +21,7 @@ use crate::prelude::*; use super::Params; +#[cfg(feature = "host")] use super::super::script::execute_commands; /// A fully-resolved script sync step: the command(s), the working directory, and @@ -96,8 +97,10 @@ pub trait ScriptRunner: Sync + Send { } /// The [`ScriptRunner`] that spawns each command as a host subprocess. +#[cfg(feature = "host")] pub struct HostScripts; +#[cfg(feature = "host")] #[async_trait] impl ScriptRunner for HostScripts { async fn run_script( diff --git a/crates/icp-project/src/host.rs b/crates/icp-project/src/host.rs index a432a0392..df5971078 100644 --- a/crates/icp-project/src/host.rs +++ b/crates/icp-project/src/host.rs @@ -77,6 +77,9 @@ pub struct Host { /// Network resolution: endpoints, root keys, friendly domains pub network: Arc, + /// Source of randomness, for picking a subnet out of several + pub random: Arc, + /// Where to report what resolution turned up. See [`Observe`]. pub observer: Arc, } @@ -117,6 +120,7 @@ impl Host { syncer: Arc::new(crate::canister::sync::UnimplementedMockSyncer), wasm: Arc::new(crate::canister::wasm::UnimplementedMockFetch), network: Arc::new(crate::network::MockNetworkAccessor::new()), + random: Arc::new(crate::random::FirstChoice), observer: Arc::new(Ignore), } } diff --git a/crates/icp-project/src/lib.rs b/crates/icp-project/src/lib.rs index 29d109d8b..02d6aaf9d 100644 --- a/crates/icp-project/src/lib.rs +++ b/crates/icp-project/src/lib.rs @@ -39,6 +39,7 @@ pub mod operations; pub mod parsers; pub mod prelude; pub mod project; +pub mod random; pub mod signal; pub mod store_artifact; pub mod store_id; diff --git a/crates/icp-project/src/operations/bundle.rs b/crates/icp-project/src/operations/bundle.rs index 43ed39bc8..e561f9641 100644 --- a/crates/icp-project/src/operations/bundle.rs +++ b/crates/icp-project/src/operations/bundle.rs @@ -379,6 +379,7 @@ impl Pruned<'_> { } } +/// Assemble a bundle from a workspace and write it out as a `.tar.gz`. pub async fn create_bundle( files: &dyn FileSystem, project_dir: &Path, diff --git a/crates/icp-project/src/operations/create.rs b/crates/icp-project/src/operations/create.rs index 42b065e57..b4bd41e51 100644 --- a/crates/icp-project/src/operations/create.rs +++ b/crates/icp-project/src/operations/create.rs @@ -27,7 +27,6 @@ use icp_canister_interfaces::{ }, icp_ledger::{ICP_LEDGER_BLOCK_FEE_E8S, ICP_LEDGER_PRINCIPAL}, }; -use rand::seq::IndexedRandom; use snafu::{OptionExt, ResultExt, Snafu}; use tokio::{select, sync::OnceCell, time::sleep}; use tracing::{info, warn}; @@ -175,6 +174,7 @@ pub enum CreateTarget { struct CreateOperationInner { calls: Arc, + random: Arc, target: CreateTarget, funding: CreateFunding, existing_canisters: Vec, @@ -196,6 +196,7 @@ impl Clone for CreateOperation { impl CreateOperation { pub fn new( calls: Arc, + random: Arc, target: CreateTarget, funding: CreateFunding, existing_canisters: Vec, @@ -203,6 +204,7 @@ impl CreateOperation { Self { inner: Arc::new(CreateOperationInner { calls, + random, target, funding, existing_canisters, @@ -647,9 +649,14 @@ impl CreateOperation { .await .map_err(|e| e.to_string())?; - subnets - .choose(&mut rand::rng()) - .copied() + let chosen = self + .inner + .random + .index_below(subnets.len()) + .await + .map_err(|e| e.to_string())?; + chosen + .and_then(|i| subnets.get(i).copied()) .ok_or_else(|| "no available subnets found".to_string()) } }) diff --git a/crates/icp-project/src/operations/deploy.rs b/crates/icp-project/src/operations/deploy.rs index 4c23cc8bf..bfbda4146 100644 --- a/crates/icp-project/src/operations/deploy.rs +++ b/crates/icp-project/src/operations/deploy.rs @@ -433,6 +433,7 @@ async fn create_canisters( }; let create_operation = CreateOperation::new( calls.clone(), + host.random.clone(), target, CreateFunding::Cycles(params.cycles), existing_ids, diff --git a/crates/icp-project/src/operations/mod.rs b/crates/icp-project/src/operations/mod.rs index 0b444a533..3023f1e85 100644 --- a/crates/icp-project/src/operations/mod.rs +++ b/crates/icp-project/src/operations/mod.rs @@ -1,5 +1,9 @@ pub mod binding_env_vars; pub mod build; +// Host-side: a bundle is a `.tar.gz` on a disk, and a plugin's declared +// directory goes into it as a tree walked for symlinks, which no seam over +// `FileSystem` can reproduce faithfully. +#[cfg(feature = "host")] pub mod bundle; pub mod candid_compat; pub mod create; diff --git a/crates/icp-project/src/random.rs b/crates/icp-project/src/random.rs new file mode 100644 index 000000000..e62278e19 --- /dev/null +++ b/crates/icp-project/src/random.rs @@ -0,0 +1,73 @@ +//! Choosing at random. +//! +//! Only one thing here needs randomness — picking which subnet to create a +//! canister on when the manifest names a kind rather than a subnet — and it +//! cannot be done by arithmetic. On a host it is a syscall; inside a canister +//! it is a `raw_rand` call to the management canister, which is why this is +//! asked for rather than done. + +use async_trait::async_trait; +use snafu::Snafu; + +/// Randomness could not be obtained. +/// +/// Where it comes from is the implementation's business — a syscall, a +/// management-canister call — so the cause is carried whole and displayed as +/// itself. +#[derive(Debug, Snafu)] +#[snafu(display("{source}"))] +pub struct RandomError { + pub source: Box, +} + +impl RandomError { + /// Wraps an implementation's own error for the trait boundary. + pub fn new(source: impl std::error::Error + Send + Sync + 'static) -> Self { + Self { + source: Box::new(source), + } + } +} + +/// A source of randomness. +/// +/// The method is a choice rather than a byte buffer because a choice is what +/// every caller wants, and turning bytes into an unbiased index is the kind of +/// thing each caller would get subtly wrong on its own. +#[async_trait] +pub trait Random: Send + Sync { + /// A uniformly distributed index below `count`, or `None` when `count` is + /// zero and there is nothing to choose. + async fn index_below(&self, count: usize) -> Result, RandomError>; +} + +#[cfg(feature = "host")] +/// [`Random`] from the operating system's entropy. +#[derive(Debug, Default, Clone, Copy)] +pub struct HostRandom; + +#[cfg(feature = "host")] +#[async_trait] +impl Random for HostRandom { + async fn index_below(&self, count: usize) -> Result, RandomError> { + use rand::RngExt; + match count { + 0 => Ok(None), + count => Ok(Some(rand::rng().random_range(0..count))), + } + } +} + +#[cfg(any(test, feature = "test-util"))] +/// [`Random`] that always chooses the first of anything, for a test that needs +/// a choice made but not an unpredictable one. +#[derive(Debug, Default, Clone, Copy)] +pub struct FirstChoice; + +#[cfg(any(test, feature = "test-util"))] +#[async_trait] +impl Random for FirstChoice { + async fn index_below(&self, count: usize) -> Result, RandomError> { + Ok((count > 0).then_some(0)) + } +} diff --git a/crates/icp-project/src/signal.rs b/crates/icp-project/src/signal.rs index 607fba7b6..6bc508d2a 100644 --- a/crates/icp-project/src/signal.rs +++ b/crates/icp-project/src/signal.rs @@ -1,4 +1,5 @@ /// Utilities for handling process termination signals across platforms. +#[cfg(feature = "host")] use tokio::select; /// Waits for a stop signal (Ctrl+C, SIGTERM on Unix, or window close on Windows). @@ -26,7 +27,7 @@ use tokio::select; /// # } /// # async fn do_work() {} /// ``` -#[cfg(unix)] +#[cfg(all(unix, feature = "host"))] pub async fn stop_signal() { use tokio::signal::unix::{SignalKind, signal}; let mut sigterm = signal(SignalKind::terminate()).unwrap(); @@ -61,7 +62,7 @@ pub async fn stop_signal() { /// # } /// # async fn do_work() {} /// ``` -#[cfg(windows)] +#[cfg(all(windows, feature = "host"))] pub async fn stop_signal() { use tokio::signal::windows::{ctrl_break, ctrl_close}; let mut ctrl_break = ctrl_break().unwrap(); @@ -72,3 +73,13 @@ pub async fn stop_signal() { _ = ctrl_close.recv() => {}, } } + +/// Waits for a stop signal — forever, where there is no process to signal. +/// +/// A caller that races this against its own work is asking to be interrupted +/// if anything interrupts the program. Nothing can, off a host, so the arm +/// simply never fires. +#[cfg(not(feature = "host"))] +pub async fn stop_signal() { + std::future::pending().await +} diff --git a/crates/icp-sync-plugin/Cargo.toml b/crates/icp-sync-plugin/Cargo.toml index 7cdcb33df..3a8d4a08b 100644 --- a/crates/icp-sync-plugin/Cargo.toml +++ b/crates/icp-sync-plugin/Cargo.toml @@ -19,7 +19,9 @@ icp-events.workspace = true icp-project = { path = "../icp-project", default-features = false } semver.workspace = true snafu.workspace = true -tokio.workspace = true +# `rt-multi-thread` for `block_in_place`, which the runtime needs because a +# wasm import cannot suspend while a canister call is in flight. +tokio = { workspace = true, features = ["rt-multi-thread"] } wasmtime.workspace = true wasmtime-wasi.workspace = true From 1d670db5b203277460f4a0628255781adaa6f5ff Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 06:03:22 -0700 Subject: [PATCH 2/5] fix: pass the randomness seam's cause through instead of restating it `RandomError` rendered its boxed cause with `#[snafu(display("{source}"))]`, which makes the cause both the wrapper's own message and its reported source, so it prints twice in every chain it reaches. `#[snafu(transparent)]` keeps the message and drops the wrapper from the chain. Last of the seam errors carrying a boxed cause. --- crates/icp-project/src/random.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/icp-project/src/random.rs b/crates/icp-project/src/random.rs index e62278e19..6744fe21d 100644 --- a/crates/icp-project/src/random.rs +++ b/crates/icp-project/src/random.rs @@ -15,7 +15,7 @@ use snafu::Snafu; /// management-canister call — so the cause is carried whole and displayed as /// itself. #[derive(Debug, Snafu)] -#[snafu(display("{source}"))] +#[snafu(transparent)] pub struct RandomError { pub source: Box, } From 91ee7ff4b6ffab8e0e8932e34c604cfb47974aed Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 11 Sep 2026 08:51:36 -0700 Subject: [PATCH 3/5] fix: prove the core reaches no host, and fix what that turns up Building icp-project for wasm32-unknown-unknown does not show a host reach on its own: the target ships a full std whose fs, process and env backends compile and fail only at runtime, so a bare std::fs::read_to_string passes the check that was meant to reject it. What the build does leave behind is one relocatable wasm object per codegen unit, and there every symbol the crate calls but does not define is an import from the `env` module. So read those imports back and reject any that name a corner of std only a host serves. That turns up two. consolidate_manifest asked the filesystem directly whether a network or environment manifest was there, and announce_workspace_root_once canonicalized with dunce. Both go through the files seam now, which leaves dunce host-only. Alongside: - BuildError was uninhabited without `host`, so a Build implemented off this machine had no way to report a failed step but to panic. It carries a boxed cause now, like every other seam's error. - camino-tempfile was host-only, so the crate's tests did not build with host off and nothing exercised that configuration at all. It is a dev-dependency now, the tests that drive host implementations are gated, and CI runs the remaining 183 with host off. - The dependency-direction guard read grep's "could not read the file" as "found nothing", so it would have gone on reporting success after a rename moved one of the paths it searches. - A Random implementation returning an out-of-range index was reported as "no available subnets found", hiding the seam's bug behind a plausible user-facing message. --- .claude/CLAUDE.md | 28 +++++--- .github/workflows/checks.yml | 48 ++++++++++++-- crates/icp-project/Cargo.toml | 13 ++-- crates/icp-project/src/canister/build/mod.rs | 18 ++++++ .../icp-project/src/canister/sync/plugin.rs | 2 + .../icp-project/src/canister/sync/script.rs | 4 ++ crates/icp-project/src/lib.rs | 13 ++-- crates/icp-project/src/manifest/mod.rs | 3 +- crates/icp-project/src/operations/create.rs | 16 ++++- crates/icp-project/src/project.rs | 4 +- crates/icp-project/src/store_artifact.rs | 2 +- scripts/check-no-host-reach.sh | 64 +++++++++++++++++++ 12 files changed, 183 insertions(+), 32 deletions(-) create mode 100755 scripts/check-no-host-reach.sh diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index c68913f62..069ce02f8 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -14,6 +14,8 @@ cargo test --test -- # Specific test cargo fmt && cargo clippy # Run after changes pass tests # `icp-project`'s core must reach nothing only a host has — see the boundary below cargo clippy -p icp-project --no-default-features --target wasm32-unknown-unknown +./scripts/check-no-host-reach.sh # ...which a build alone does not prove +cargo test -p icp-project --no-default-features ./scripts/generate-cli-docs.sh # Regenerate CLI docs when commands change ./scripts/generate-config-schemas.sh # Regenerate schema when manifest types change ``` @@ -66,14 +68,24 @@ default-on **`host`** feature: `HostFileSystem`, the two stores, `HostScripts`, `HostRandom`, and `Builder`. The rest have none there at all. `host` must be **strictly additive** — it may add implementations, never change -what the rest of the crate does. CI checks the core with -`cargo clippy -p icp-project --no-default-features --target -wasm32-unknown-unknown`, on a target where reaching the host does not compile; -if `host` changed behaviour rather than adding to it, that check would prove -nothing about the shipped binary. Whole modules that are irreducibly host-side -sit behind it too — `operations::bundle` writes a `.tar.gz` and walks a -directory tree for symlinks, which no seam over `FileSystem` reproduces -faithfully. +what the rest of the crate does; if it changed behaviour rather than adding to +it, the checks below would prove nothing about the shipped binary. CI runs +three of them on the core with default features off: + +- a build for `wasm32-unknown-unknown`, which rejects a *dependency* that + reaches the host, because such a crate gates that code on + `cfg(unix)`/`cfg(windows)` and is left with nothing to compile; +- `scripts/check-no-host-reach.sh`, which reads the imports back out of that + build's object files and rejects a call this crate itself makes into + `std::fs`, `std::process`, `std::env` and their neighbours. The build alone + does not catch those: `wasm32-unknown-unknown` ships a full `std` whose host + backends compile and fail at runtime; +- the crate's own tests, which are the only thing that exercises it with + `host` off, and so run in that configuration too. + +Whole modules that are irreducibly host-side sit behind it too — +`operations::bundle` writes a `.tar.gz` and walks a directory tree for +symlinks, which no seam over `FileSystem` reproduces faithfully. Because those are implemented across a crate boundary, their error types carry their cause boxed and pass it through with `#[snafu(transparent)]`, which leaves diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 5a586f4d9..cf486dc75 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -87,8 +87,8 @@ jobs: # `icp-project` is meant to end up runnable inside a canister, so its core # must reach nothing that only a host has. Nothing else expresses this: a # `--workspace` build unifies the `host` feature back on, because `icp-cli` - # enables it. So this builds that one crate, alone, for a target where - # reaching the host does not compile. + # enables it. So this builds that one crate, alone, for the target it would + # run on, and then reads back what the build left behind. canister-target: name: canister-target:required needs: [changes, compile] @@ -110,9 +110,15 @@ jobs: - uses: actions-rust-lang/setup-rust-toolchain@150fca883cd4034361b621bd4e6a9d34e5143606 # v1.15.4 with: target: wasm32-unknown-unknown + # `llvm-nm` reads the imports back out of the objects the build + # leaves behind; see scripts/check-no-host-reach.sh. + components: llvm-tools cache-shared-key: ${{ runner.os }}-canister-target cache-bin: false + # Catches a dependency that reaches the host, which fails to compile + # here. Only the library: the dev-dependencies behind the tests serve a + # host by definition, and `httptest` does not build for wasm at all. - name: Check icp-project without a host run: | cargo clippy -p icp-project --no-default-features \ @@ -120,17 +126,45 @@ jobs: env: RUST_BACKTRACE: 1 - # The check above proves the core reaches no host. This proves it reaches + # Catches the crate's own code reaching the host, which compiles here: + # `wasm32-unknown-unknown` ships a full `std` whose host backends fail at + # runtime rather than at build time. + - name: Check the core reaches no host + run: ./scripts/check-no-host-reach.sh + + # The tests are the only thing that exercises the crate with `host` off, + # so they run in that configuration too — on this machine's target, + # since that is where a test can run at all. + - name: Test icp-project without a host + run: cargo test -p icp-project --no-default-features + env: + RUST_BACKTRACE: 1 + + # The checks above prove the core reaches no host. This proves it reaches # no *crate above it* either, which no build would catch, because a cycle # is what cargo would report and there is no cycle to report until # someone writes one. - name: Check the dependency direction run: | fail() { echo "::error::$1"; exit 1; } - grep -n 'icp-app' crates/icp-project/Cargo.toml crates/icp-sync-plugin/Cargo.toml \ - && fail "icp-project and icp-sync-plugin must not depend on icp-app" - grep -rn 'pub use icp_project' crates/icp-app/src \ - && fail "icp-app must not re-export icp-project: a command reaches for the crate that owns what it needs" + # grep exits 1 when it found nothing and 2 or more when it could not + # read a file. Only the first is a pass: a guard that quietly stops + # guarding once a path moves is worse than no guard at all. + forbid() { + local message=$1 pattern=$2 + shift 2 + local matches status + matches=$(grep -rn "$pattern" "$@") && status=0 || status=$? + case $status in + 0) echo "$matches"; fail "$message" ;; + 1) ;; + *) fail "could not search $* for '$pattern'" ;; + esac + } + forbid "icp-project and icp-sync-plugin must not depend on icp-app" \ + 'icp-app' crates/icp-project/Cargo.toml crates/icp-sync-plugin/Cargo.toml + forbid "icp-app must not re-export icp-project: a command reaches for the crate that owns what it needs" \ + 'pub use icp_project' crates/icp-app/src echo "the boundary holds" format: diff --git a/crates/icp-project/Cargo.toml b/crates/icp-project/Cargo.toml index a7feded51..70abb7dfd 100644 --- a/crates/icp-project/Cargo.toml +++ b/crates/icp-project/Cargo.toml @@ -11,11 +11,13 @@ default = ["host"] # the filesystem, the project-local `.icp` stores, subprocesses, randomness. # Turned off for a build that has to run somewhere without them, such as inside # a canister — which CI checks by building this crate for -# `wasm32-unknown-unknown` with default features off. +# `wasm32-unknown-unknown` with default features off, reading the host calls +# back out of that build with `scripts/check-no-host-reach.sh`, and running the +# tests in the same configuration. # # Strictly additive: it may add implementations, never change what the rest of -# the crate does, or the check proves nothing about the shipped binary. -host = ["dep:camino-tempfile", "dep:rand", "dep:tar", "tokio/io-std", "tokio/process", "tokio/signal"] +# the crate does, or those checks prove nothing about the shipped binary. +host = ["dep:camino-tempfile", "dep:dunce", "dep:rand", "dep:tar", "tokio/io-std", "tokio/process", "tokio/signal"] # Exposes this crate's mocks and fixtures so downstream crates can test against # the same seams. Off in a normal build, so none of it ships. test-util = [] @@ -28,7 +30,7 @@ camino-tempfile = { workspace = true, optional = true } candid = { workspace = true } candid_parser = { workspace = true } clap = { workspace = true, optional = true } -dunce = { workspace = true } +dunce = { workspace = true, optional = true } flate2 = { workspace = true } futures = { workspace = true } glob = { workspace = true } @@ -69,5 +71,8 @@ wasmparser = { workspace = true } winreg = { workspace = true } [dev-dependencies] +# Also an optional dependency above, for `host`'s `scratch_dir`. Unconditional +# here so the tests build with `host` off, which is the configuration CI checks. +camino-tempfile = { workspace = true } httptest = { workspace = true } jsonschema = { workspace = true } diff --git a/crates/icp-project/src/canister/build/mod.rs b/crates/icp-project/src/canister/build/mod.rs index 534de549b..b51deec45 100644 --- a/crates/icp-project/src/canister/build/mod.rs +++ b/crates/icp-project/src/canister/build/mod.rs @@ -31,6 +31,24 @@ pub enum BuildError { #[cfg(feature = "host")] #[snafu(transparent)] Prebuilt { source: prebuilt::PrebuiltError }, + /// A [`Build`] this crate cannot name failed. How it runs a step is its own + /// business, so the cause is carried whole and displayed as itself. + /// + /// This is the only variant a build off this machine has, and without it + /// the enum would be uninhabited there. + #[snafu(transparent)] + Other { + source: Box, + }, +} + +impl BuildError { + /// Wraps an implementation's own error for the trait boundary. + pub fn new(source: impl std::error::Error + Send + Sync + 'static) -> Self { + Self::Other { + source: Box::new(source), + } + } } #[async_trait] diff --git a/crates/icp-project/src/canister/sync/plugin.rs b/crates/icp-project/src/canister/sync/plugin.rs index 75edd7138..e91fac91c 100644 --- a/crates/icp-project/src/canister/sync/plugin.rs +++ b/crates/icp-project/src/canister/sync/plugin.rs @@ -365,6 +365,7 @@ fn resolve_callable( mod tests { use super::*; + #[cfg(feature = "host")] #[test] fn parse_compute_limit_accepts_positive_integers() { assert_eq!(parse_compute_limit("300").unwrap(), 300); @@ -372,6 +373,7 @@ mod tests { assert_eq!(parse_compute_limit(" 42 ").unwrap(), 42); } + #[cfg(feature = "host")] #[test] fn parse_compute_limit_rejects_invalid_values() { for bad in ["0", "abc", "30O", "-5", "1.5", ""] { diff --git a/crates/icp-project/src/canister/sync/script.rs b/crates/icp-project/src/canister/sync/script.rs index 3a77fca21..b278fc5bb 100644 --- a/crates/icp-project/src/canister/sync/script.rs +++ b/crates/icp-project/src/canister/sync/script.rs @@ -139,6 +139,7 @@ mod tests { /// Serializes the tests here that mutate the process environment, since /// cargo runs tests in parallel threads. Async-aware because the variable /// has to stay set across the subprocess `await` that reads it. + #[cfg(feature = "host")] static ENV_MUTEX: Mutex<()> = Mutex::const_new(()); fn principal(byte: u8) -> Principal { @@ -209,6 +210,7 @@ mod tests { } /// The host runner passes the resolved environment through to the subprocess. + #[cfg(feature = "host")] #[tokio::test] async fn host_runner_applies_the_resolved_environment() { let out = camino_tempfile::NamedUtf8TempFile::new().unwrap(); @@ -237,6 +239,7 @@ mod tests { /// sets rather than `PATH`, because Git-for-Windows bash rewrites `PATH` /// into POSIX form, so its value there never equals the `PATH` the Rust side /// reads. + #[cfg(feature = "host")] #[tokio::test] async fn host_runner_overlays_rather_than_replaces_the_environment() { const AMBIENT: &str = "ICP_CLI_TEST_AMBIENT_VAR"; @@ -281,6 +284,7 @@ mod tests { /// A command that exits non-zero surfaces as a `ScriptRunError` whose source /// still names the command and its status, so the `caused by:` line the CLI /// prints stays specific. + #[cfg(feature = "host")] #[tokio::test] async fn host_runner_reports_a_failing_command() { let invocation = ScriptInvocation { diff --git a/crates/icp-project/src/lib.rs b/crates/icp-project/src/lib.rs index 02d6aaf9d..ec17458fc 100644 --- a/crates/icp-project/src/lib.rs +++ b/crates/icp-project/src/lib.rs @@ -250,12 +250,12 @@ static WORKSPACE_ROOT_ANNOUNCED: std::sync::atomic::AtomicBool = /// Warn once when the resolved workspace root differs from the sub-project the /// command is run in, so the upward resolution (§workspace model) is visible for /// every command, not just deploy. -fn announce_workspace_root_once(member: &Path, root: &Path) { +async fn announce_workspace_root_once(files: &dyn FileSystem, member: &Path, root: &Path) { let differs = match ( - dunce::canonicalize(member.as_std_path()), - dunce::canonicalize(root.as_std_path()), + files.canonicalize(member).await, + files.canonicalize(root).await, ) { - (Ok(m), Ok(r)) => m != r, + (Some(m), Some(r)) => m != r, _ => member != root, }; if differs && !WORKSPACE_ROOT_ANNOUNCED.swap(true, std::sync::atomic::Ordering::Relaxed) { @@ -278,7 +278,7 @@ impl ProjectLoad for ProjectLoadImpl { // Announce (once) when we resolved up to a workspace root above the // sub-project the command is run in, so this is visible for every command. if let Ok(member) = self.project_root_locate.locate_member() { - announce_workspace_root_once(&member, &pdir); + announce_workspace_root_once(self.files.as_ref(), &member, &pdir).await; } // Load project manifest @@ -707,7 +707,8 @@ impl ProjectLoad for NoProjectLoader { } } -#[cfg(test)] +// Every test here loads a project off a real directory through `HostFileSystem`. +#[cfg(all(test, feature = "host"))] mod tests { use super::*; use crate::canister::recipe::{Fetched, Resolve, ResolveError}; diff --git a/crates/icp-project/src/manifest/mod.rs b/crates/icp-project/src/manifest/mod.rs index 68e7ed7b1..e687b2bc3 100644 --- a/crates/icp-project/src/manifest/mod.rs +++ b/crates/icp-project/src/manifest/mod.rs @@ -321,7 +321,8 @@ where Ok(m) } -#[cfg(test)] +// Every test here drives `ProjectRootLocateImpl` over real directories. +#[cfg(all(test, feature = "host"))] mod tests { use super::*; use camino_tempfile::Utf8TempDir; diff --git a/crates/icp-project/src/operations/create.rs b/crates/icp-project/src/operations/create.rs index b4bd41e51..b81cd803f 100644 --- a/crates/icp-project/src/operations/create.rs +++ b/crates/icp-project/src/operations/create.rs @@ -648,6 +648,9 @@ impl CreateOperation { let subnets = get_available_subnets(self.inner.calls.as_ref()) .await .map_err(|e| e.to_string())?; + if subnets.is_empty() { + return Err("no available subnets found".to_string()); + } let chosen = self .inner @@ -655,9 +658,16 @@ impl CreateOperation { .index_below(subnets.len()) .await .map_err(|e| e.to_string())?; - chosen - .and_then(|i| subnets.get(i).copied()) - .ok_or_else(|| "no available subnets found".to_string()) + // `index_below` promises an index below the count, and the + // count is not zero, so a miss is the seam's bug and says so + // rather than posing as an empty list. + chosen.and_then(|i| subnets.get(i).copied()).ok_or_else(|| { + format!( + "randomness chose {chosen:?}, which is not one of the {} \ + available subnets", + subnets.len() + ) + }) } }) .await; diff --git a/crates/icp-project/src/project.rs b/crates/icp-project/src/project.rs index 8f6085693..289df34fb 100644 --- a/crates/icp-project/src/project.rs +++ b/crates/icp-project/src/project.rs @@ -1672,7 +1672,7 @@ pub async fn consolidate_manifest( }) } -#[cfg(test)] +#[cfg(all(test, feature = "host"))] mod recipe_sync_tests { use super::*; use crate::canister::recipe::{Fetched, Resolve, ResolveError}; @@ -1780,7 +1780,7 @@ mod recipe_sync_tests { } } -#[cfg(test)] +#[cfg(all(test, feature = "host"))] mod dependency_tests { use super::*; use crate::canister::recipe::{Fetched, Resolve, ResolveError}; diff --git a/crates/icp-project/src/store_artifact.rs b/crates/icp-project/src/store_artifact.rs index 402fadc3c..707fe734c 100644 --- a/crates/icp-project/src/store_artifact.rs +++ b/crates/icp-project/src/store_artifact.rs @@ -261,7 +261,7 @@ impl Access for MockInMemoryArtifactStore { } } -#[cfg(test)] +#[cfg(all(test, feature = "host"))] mod tests { use super::{artifact_name_overflow, sanitize_artifact_name}; diff --git a/scripts/check-no-host-reach.sh b/scripts/check-no-host-reach.sh new file mode 100755 index 000000000..e8b6391a2 --- /dev/null +++ b/scripts/check-no-host-reach.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# +# Fails if `icp-project`'s core reaches for something only a host has. +# +# Building the crate for `wasm32-unknown-unknown` does not prove this on its +# own: that target ships a full `std` whose `fs`, `process` and `env` backends +# compile and fail only at runtime, so a bare `std::fs::read_to_string` passes +# a build that was meant to reject it. What the build does leave behind is one +# relocatable wasm object per codegen unit, and there every symbol the crate +# calls but does not define is an import from the `env` module. So read those +# imports back and fail on any that names a corner of `std` only a host serves. +# +# This sees what `icp-project` itself calls, including whatever a dependency's +# generics monomorphise into it. A dependency reaching the host from its own +# objects is the build's job to reject, which it does whenever that crate gates +# the code on `cfg(unix)`/`cfg(windows)` — as `tokio`'s `process` and `signal` +# do, and as `rand`'s entropy source does. + +set -euo pipefail + +cd "$(dirname "$0")/.." + +# Paths in `std` that only a host can serve, written as `llvm-nm` demangles +# them. `std::sys` is std's own platform layer, listed because a call that +# arrives there from an inlined front door would otherwise go unseen. +denied='std::(fs|net|os|env|process|time)::' +denied+='|std::io::stdio::' +denied+='|std::sys::(args|env|fd|fs|net|os|pal|process|stdio|thread|time)' +# Not a host reach: `abort` is how a panic while unwinding gives up, and wasm +# has the instruction for it. +allowed='std::process::abort' + +rlib=$( + cargo build -p icp-project --no-default-features \ + --target wasm32-unknown-unknown --message-format=json | + jq -r 'select(.reason == "compiler-artifact" and .target.name == "icp_project") + | .filenames[] | select(endswith(".rlib"))' | + tail -1 +) +if [[ -z $rlib ]]; then + echo "::error::cargo produced no icp-project rlib to inspect" >&2 + exit 1 +fi + +nm=$(find "$(rustc --print sysroot)" -name 'llvm-nm*' -type f -print -quit) +if [[ -z $nm ]]; then + echo "::error::llvm-nm not found; run 'rustup component add llvm-tools'" >&2 + exit 1 +fi + +# An rlib also carries cargo's metadata members, which `llvm-nm` reports as +# holding no symbols; that is expected, so keep it out of the log. +symbols=$("$nm" --demangle --undefined-only "$rlib" 2> >(grep -v ': no symbols$' >&2)) + +reaches=$(printf '%s\n' "$symbols" | sed 's/^ *U //' | sort -u | + grep -E "$denied" | grep -vxE "$allowed" || true) + +if [[ -n $reaches ]]; then + echo "::error::icp-project's core reaches the host; it must ask through a seam instead" + echo "$reaches" | sed 's/^/ /' + exit 1 +fi + +echo "icp-project's core reaches no host" From 4339254c39f6a5ada30f356d69bb87e0d6df003c Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Mon, 14 Sep 2026 08:16:12 -0700 Subject: [PATCH 4/5] fix: deny the host front doors a dependency offers too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The symbol scan reads the imports out of `icp-project`'s own objects, so it sees a host call this crate makes and a host call a dependency's generics monomorphise into it — but not one made from a dependency's own non-generic code, which is compiled once into that dependency's objects. Such a call arrives in these imports named after the dependency, and matches nothing in `std`: put a `path.is_file()` back into `project.rs` and the crate imports `::is_file`, with the `std::fs::metadata` behind it sitting in camino's archive, unscanned. The header claimed the build covers that case, which holds only for a dependency that gates the code on `cfg(unix)`/`cfg(windows)` and is left with nothing to compile. Scanning the whole closure is no way out: `camino` is a single codegen unit that reaches `std::sys::fs`, so any reference into it would flag. So deny the front doors by name instead, over the crates that can mediate a reach at all — found by reading these same imports out of every rlib in the closure rather than guessed. That turns one up. `operations::create` asked `icp_canister_interfaces::engine_canister::engine_canister_id()` which engine-canister registry to query, and that reads `ENGINE_CANISTER_ID` from the environment. The registry principal is now resolved by the caller and passed in, on `CreateOperation::new` and in `DeployParams`, which also means an invalid override is reported when the command starts rather than only once a `CloudEngine` subnet is reached. --- .claude/CLAUDE.md | 12 ++++---- .../icp-cli/src/commands/canister/create.rs | 3 ++ crates/icp-cli/src/commands/deploy.rs | 4 ++- crates/icp-project/src/operations/create.rs | 14 +++++---- crates/icp-project/src/operations/deploy.rs | 4 +++ scripts/check-no-host-reach.sh | 29 ++++++++++++++++--- 6 files changed, 50 insertions(+), 16 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 069ce02f8..540c504ba 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -73,13 +73,15 @@ it, the checks below would prove nothing about the shipped binary. CI runs three of them on the core with default features off: - a build for `wasm32-unknown-unknown`, which rejects a *dependency* that - reaches the host, because such a crate gates that code on - `cfg(unix)`/`cfg(windows)` and is left with nothing to compile; + reaches the host by gating that code on `cfg(unix)`/`cfg(windows)`, leaving + it with nothing to compile; - `scripts/check-no-host-reach.sh`, which reads the imports back out of that build's object files and rejects a call this crate itself makes into - `std::fs`, `std::process`, `std::env` and their neighbours. The build alone - does not catch those: `wasm32-unknown-unknown` ships a full `std` whose host - backends compile and fail at runtime; + `std::fs`, `std::process`, `std::env` and their neighbours — and, since a + dependency that reaches the host ungated compiles fine, the front doors onto + those that such a dependency offers, `camino`'s `Utf8Path::is_file` among + them. The build alone catches neither: `wasm32-unknown-unknown` ships a full + `std` whose host backends compile and fail at runtime; - the crate's own tests, which are the only thing that exercises it with `host` off, and so run in that configuration too. diff --git a/crates/icp-cli/src/commands/canister/create.rs b/crates/icp-cli/src/commands/canister/create.rs index 2280ccd2b..a4d533a0d 100644 --- a/crates/icp-cli/src/commands/canister/create.rs +++ b/crates/icp-cli/src/commands/canister/create.rs @@ -7,6 +7,7 @@ use clap::{ArgGroup, Args, Parser}; use ic_management_canister_types::CanisterSettings as MgmtCanisterSettings; use icp_app::context::{Context, NetworkSelection}; use icp_app::identity::IdentitySelection; +use icp_canister_interfaces::engine_canister::engine_canister_id; use icp_project::canister::resolve_controllers; use icp_project::host::EnvironmentSelection; use icp_project::parsers::{CyclesAmount, DurationAmount, MemoryAmount, parse_token_amount}; @@ -313,6 +314,7 @@ async fn create_canister(ctx: &Context, args: &CreateArgs) -> Result<(), anyhow: args.create_target(), args.funding(), vec![], + engine_canister_id().map_err(|message| anyhow!(message))?, ); let canister_settings = args.canister_settings(); @@ -385,6 +387,7 @@ async fn create_project_canister(ctx: &Context, args: &CreateArgs) -> Result<(), args.create_target(), args.funding(), ids.values().copied().collect(), + engine_canister_id().map_err(|message| anyhow!(message))?, ); let (canister_settings, unresolved) = diff --git a/crates/icp-cli/src/commands/deploy.rs b/crates/icp-cli/src/commands/deploy.rs index ffe2c5692..5d57438b0 100644 --- a/crates/icp-cli/src/commands/deploy.rs +++ b/crates/icp-cli/src/commands/deploy.rs @@ -1,9 +1,10 @@ -use anyhow::bail; +use anyhow::{anyhow, bail}; use candid::Principal; use clap::Args; use clap_complete::ArgValueCandidates; use icp_app::{context::Context, identity::IdentitySelection}; use icp_canister_interfaces::candid_ui::MAINNET_CANDID_UI_CID; +use icp_canister_interfaces::engine_canister::engine_canister_id; use icp_project::calls::{Call, CallError, CanisterCalls}; use icp_project::operations::deploy::{DeployParams, DeployReport, deploy, resolve_targets}; use icp_project::parsers::CyclesAmount; @@ -135,6 +136,7 @@ pub(crate) async fn exec(ctx: &Context, args: &DeployArgs) -> Result<(), anyhow: subnet: args.subnet, proxy: args.proxy, cycles: args.cycles.get(), + engine_registry: engine_canister_id().map_err(|message| anyhow!(message))?, no_create: args.no_create, yes: args.yes, args: args.args_opt.resolve_bytes()?, diff --git a/crates/icp-project/src/operations/create.rs b/crates/icp-project/src/operations/create.rs index b81cd803f..eefe8b036 100644 --- a/crates/icp-project/src/operations/create.rs +++ b/crates/icp-project/src/operations/create.rs @@ -23,7 +23,7 @@ use icp_canister_interfaces::{ }, engine_canister::{ GET_ENGINE_OPERATOR_BY_SUBNET_METHOD, GetEngineOperatorBySubnetArgs, - GetEngineOperatorBySubnetResult, engine_canister_id, + GetEngineOperatorBySubnetResult, }, icp_ledger::{ICP_LEDGER_BLOCK_FEE_E8S, ICP_LEDGER_PRINCIPAL}, }; @@ -66,9 +66,6 @@ pub enum CreateOperationError { subnet: Principal, }, - #[snafu(display("invalid engine-canister id: {message}"))] - EngineCanisterId { message: String }, - #[snafu(display("failed to query the engine-canister registry"))] EngineCanisterQuery { source: crate::calls::CallError }, @@ -178,6 +175,10 @@ struct CreateOperationInner { target: CreateTarget, funding: CreateFunding, existing_canisters: Vec, + /// The engine-canister registry to ask which engine-operator serves a + /// subnet. Resolved by the caller, which is where an override of it — an + /// environment variable — is something to read at all. + engine_registry: Principal, resolved_subnet: OnceCell>, } @@ -200,6 +201,7 @@ impl CreateOperation { target: CreateTarget, funding: CreateFunding, existing_canisters: Vec, + engine_registry: Principal, ) -> Self { Self { inner: Arc::new(CreateOperationInner { @@ -208,6 +210,7 @@ impl CreateOperation { target, funding, existing_canisters, + engine_registry, resolved_subnet: OnceCell::new(), }), } @@ -335,8 +338,7 @@ impl CreateOperation { &self, subnet: Principal, ) -> Result { - let engine_registry = engine_canister_id() - .map_err(|message| CreateOperationError::EngineCanisterId { message })?; + let engine_registry = self.inner.engine_registry; let arg = GetEngineOperatorBySubnetArgs { subnet_id: Some(subnet), diff --git a/crates/icp-project/src/operations/deploy.rs b/crates/icp-project/src/operations/deploy.rs index bfbda4146..0c52ae9e6 100644 --- a/crates/icp-project/src/operations/deploy.rs +++ b/crates/icp-project/src/operations/deploy.rs @@ -184,6 +184,9 @@ pub struct DeployParams { pub subnet: Option, pub proxy: Option, pub cycles: u128, + /// The engine-canister registry a created canister's subnet is looked up + /// in, already resolved from the environment by the caller. + pub engine_registry: Principal, pub no_create: bool, /// Skip the Candid interface compatibility check. pub yes: bool, @@ -437,6 +440,7 @@ async fn create_canisters( target, CreateFunding::Cycles(params.cycles), existing_ids, + params.engine_registry, ); let mut futs = FuturesOrdered::new(); diff --git a/scripts/check-no-host-reach.sh b/scripts/check-no-host-reach.sh index e8b6391a2..84c0e1be5 100755 --- a/scripts/check-no-host-reach.sh +++ b/scripts/check-no-host-reach.sh @@ -11,10 +11,12 @@ # imports back and fail on any that names a corner of `std` only a host serves. # # This sees what `icp-project` itself calls, including whatever a dependency's -# generics monomorphise into it. A dependency reaching the host from its own -# objects is the build's job to reject, which it does whenever that crate gates -# the code on `cfg(unix)`/`cfg(windows)` — as `tokio`'s `process` and `signal` -# do, and as `rand`'s entropy source does. +# generics monomorphise into it. What it cannot see is a dependency's own +# non-generic code, compiled once into that dependency's objects: a call that +# only arrives at the host a frame or two inside one of those is named here +# after the dependency rather than after `std`, and matches nothing in `std`. +# `camino`'s `Utf8Path::is_file` is such a front door, and the one this crate +# would reach for by accident, so those are denied by name too. set -euo pipefail @@ -26,6 +28,25 @@ cd "$(dirname "$0")/.." denied='std::(fs|net|os|env|process|time)::' denied+='|std::io::stdio::' denied+='|std::sys::(args|env|fd|fs|net|os|pal|process|stdio|thread|time)' + +# A dependency's own front door onto one of those. Found by reading these same +# imports out of every rlib in the `--no-default-features` closure and keeping +# the crates whose own objects reach a host corner of `std`: `camino`, `glob`, +# `handlebars`, `candid_parser`, `icp-canister-interfaces` and `snafu`. Every +# other crate there reaches the host only where the build already rejects it, +# gating the code on `cfg(unix)`/`cfg(windows)` — as `tokio`'s `process` and +# `signal` do, and as `rand`'s entropy source does. `snafu` earns no entry: its +# one reach is the `RUST_LIB_BACKTRACE` a captured `Backtrace` reads, and no +# error in this repo carries one. +denied+='|<(std::path::Path|camino::Utf8Path|camino::Utf8DirEntry)>::' +denied+='(try_exists|exists|is_file|is_dir|is_symlink|metadata|symlink_metadata' +denied+='|canonicalize|read_dir|read_link|file_type)' +denied+='| Date: Mon, 14 Sep 2026 09:44:09 -0700 Subject: [PATCH 5/5] fix: gate the test mutex's import with the mutex itself ENV_MUTEX serializes the script tests that mutate the process environment, and those tests only exist with `host`, so the static was gated but the `tokio::sync::Mutex` import above it was not. With `host` off the import has no user left, and CI builds that configuration with `-D warnings`. --- crates/icp-project/src/canister/sync/script.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/icp-project/src/canister/sync/script.rs b/crates/icp-project/src/canister/sync/script.rs index b278fc5bb..d0328393d 100644 --- a/crates/icp-project/src/canister/sync/script.rs +++ b/crates/icp-project/src/canister/sync/script.rs @@ -128,6 +128,7 @@ impl ScriptRunner for HostScripts { mod tests { use std::collections::BTreeMap; + #[cfg(feature = "host")] use tokio::sync::Mutex; use candid::Principal;