refactor: Abstract out FS access from icp-project - #773
Open
adamspofford-dfinity wants to merge 12 commits into
Open
adamspofford-dfinity wants to merge 12 commits into
adamspofford-dfinity wants to merge 12 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several project validations and bundle paths still bypass the abstraction, while recursive glob traversal can loop through symlink cycles.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Introduces a FileSystem abstraction so project loading, builds, and bundles can support non-host storage backends.
Changes:
- Adds asynchronous filesystem and scratch-directory interfaces.
- Routes manifest, build, bundle, and host operations through the new abstraction.
- Adds host feature-gating and generalized store errors.
File summaries
| File | Description |
|---|---|
crates/icp-project/src/files.rs |
Adds filesystem abstractions and glob expansion. |
crates/icp-project/src/fs/mod.rs |
Adds host directory listing. |
crates/icp-project/src/project.rs |
Abstracts project file access. |
crates/icp-project/src/manifest/mod.rs |
Abstracts manifest reads. |
crates/icp-project/src/operations/build.rs |
Uses abstract scratch storage. |
crates/icp-project/src/operations/bundle.rs |
Abstracts bundle input access. |
crates/icp-project/src/operations/deploy.rs |
Supplies the host filesystem. |
crates/icp-project/src/store_id.rs |
Gates host storage and generalizes errors. |
crates/icp-project/src/store_artifact.rs |
Gates host artifact storage. |
crates/icp-project/src/canister/build/mod.rs |
Injects filesystem access into builders. |
crates/icp-project/src/canister/build/prebuilt.rs |
Abstracts WASM copying. |
crates/icp-project/src/host.rs |
Adds filesystem access to Host. |
crates/icp-project/src/lib.rs |
Exposes and wires the abstraction. |
crates/icp-project/Cargo.toml |
Adds the default host feature. |
crates/icp-cli/src/commands/build.rs |
Passes filesystem access to builds. |
crates/icp-cli/src/commands/project/bundle.rs |
Passes filesystem access to bundling. |
crates/icp-app/src/context/init.rs |
Initializes the host filesystem implementation. |
Review details
Suppressed comments (5)
crates/icp-project/src/project.rs:703
- Dependency discovery still checks the machine filesystem at line 710 before using the injected filesystem.
workspace_instanceswill therefore reject dependencies that exist only in a customFileSystem.
async fn resolve_edges(
files: &dyn FileSystem,
dir: &Path,
crates/icp-project/src/project.rs:930
- This dependency import remains gated by
manifest_path.is_file()on the host filesystem. A manifest supplied by the newFileSystemabstraction is rejected beforeload_manifest_from_path(files, ...)can read it.
async fn import_dependency(
files: &dyn FileSystem,
app_root_canonical: &Path,
crates/icp-project/src/project.rs:1053
- The referenced dependency environment is validated with
p.is_file()on the host at line 1045, so custom-filesystem manifests fail withNotFoundbefore this abstracted load. Usefiles.is_filefor the validation too.
load_manifest_from_path::<EnvironmentManifest>(files, &p)
.await
crates/icp-project/src/project.rs:1449
- The network manifest load is abstracted, but the preceding
exists/is_filechecks still query the machine filesystem. This guarantees a falseNotFoundfor network manifests held by anotherFileSystemimplementation.
load_manifest_from_path::<NetworkManifest>(files, &path)
.await
crates/icp-project/src/project.rs:1529
- The environment manifest load is abstracted, but its validation still queries the machine filesystem at line 1521. Virtual environment manifests are therefore rejected before they can be read through
files.
load_manifest_from_path::<EnvironmentManifest>(files, &path)
.await
- Files reviewed: 17/17 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
adamspofford-dfinity
force-pushed
the
spofford/abstract-filesystem
branch
from
September 14, 2026 14:23
0e3e739 to
9b4b700
Compare
adamspofford-dfinity
force-pushed
the
spofford/abstract-filesystem
branch
2 times, most recently
from
September 14, 2026 18:34
d137a46 to
059afe5
Compare
Consolidating a manifest reads the manifests it points at, the files its arguments and environment variables come from, and the directories its globs expand over; building reads back the module a build step produced. All of that went straight to `std::fs`, which a project loaded from somewhere other than a filesystem could never do. `files::FileSystem` is that surface now, with `HostFileSystem` behind a new default-on `host` feature. `Host` carries it, `ProjectLoadImpl` and `Builder` hold it, and `manifest::load_manifest_from_path`, `project::consolidate_manifest` and `operations::bundle::create_bundle` take it. The project-local `.icp` stores keep the traits they already had; only their implementations move behind `host`. `glob::glob` had to go: it walks the real filesystem itself, so no seam could stand in front of it. `files::expand_glob` matches a component at a time over `FileSystem::read_dir` instead, including the `**` the manifest reference documents. It also sorts each directory's entries, which the old code's own comment noted it could not do — so a bundle's canister ordering no longer depends on the order the filesystem happened to hand them back. Eight tests cover it, `**` included; the pattern it replaced had none. Three trait errors could no longer name their own source trees, since the implementation now lives on the far side of a feature gate. Each carries its cause opaquely, but the contextual fields stay — which environment's id store, which canister's artifact — so nothing the user reads is lost. `canonicalize` answers with `Option`, matching the `camino` method it replaces, so `BundleError::CanonicalizePath` no longer carries an `io::Error` it could not have. `cargo check -p icp-project --no-default-features` now passes: nothing outside the gate needs the host. That is not yet the wasm gate — every dependency is still non-optional and still linked, which is what the last stage is for — but it is the boundary the feature claims. Still host-shaped inside `icp-project`, and named here so the last stage has the list: `operations::build` makes a temp directory to build into, `ArchiveWriter::dir` uses `tar`'s own directory walk to keep symlinks as symlinks, `create.rs` draws on `rand`, and the subprocess and wasmtime step runners are untouched.
`FsError` 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.
Matches the seam errors in `network`, `canister::wasm` and
`canister::recipe`.
`expand_glob` matched every component by listing its parent and filtering
the names, and a listing never yields `..`, so any pattern containing one
matched nothing at all — silently. A monorepo whose root manifest reaches
a sibling directory (`canisters: [../shared/*]`) lost every canister it
names. An absolute pattern fared no better: splitting on `/` left a
leading empty component that was skipped, so `/opt/shared/*` walked from
the project directory instead of from the root.
Both are components that cannot match anything, so neither belongs in the
matching loop. Walking `Utf8Path::components()` instead of `split('/')`
names them: `ParentDir` is appended the way `join` appends it (and still
has to name a directory that is there), and `Prefix`/`RootDir` are pushed,
which is what discards the base — by the same rule that joining an
absolute path onto another discards the other.
That is the rule the non-glob branch of `build_manifest_canisters` has
always followed, since it is just `pdir.join(pattern)`. The two branches
now agree: a pattern with no metacharacters names the same path either
way.
Components also carry the platform's separators, so a `\`-spelled pattern
splits on Windows and stays literal elsewhere, matching what `glob::glob`
did with one.
`--no-default-features --features test-util` is the configuration the feature exists for — a build without the machine underneath it, with this crate's mocks still exposed so downstream tests can stand on the same seams — and it did not compile. `Host::mocked()` reached for `HostFileSystem`, which the `host` gate had just taken away, and the `store_id` mock used a `Mutex` whose import had moved behind the same gate. `files::UnimplementedMockFileSystem` is the seam's mock, alongside the ones `build`, `sync` and `wasm` already have; `Host::mocked()` takes it, so a mocked host is now mocks all the way down rather than the real filesystem in one field. Nothing that uses it touches files — every caller loads from `MockProjectLoader` — so the panic is the right answer if one ever starts. The `store_id` mock imports its own `Mutex` rather than borrowing the host implementation's, and the imports that only host code uses are gated, so the hostless build is warning-free too. Also drops a `#[cfg(feature = "host")]` that `artifact_name_overflow` carried twice.
`store_err` took a `&dyn Display` and rebuilt the cause as
`io::Error::other(e.to_string())`, which keeps the top frame's message and
drops everything under it. The store writes through `fs::write`, whose
error displays as "Filesystem operation failed at {path}" and carries the
real `io::Error` as its source — so a permission-denied or out-of-space
artifact write reported the path and never the reason.
It also printed that one surviving message twice: `StoreCause` displays as
its inner error but chains to that error's *source*, and for
`io::Error::other(String)` the source is a string error with the identical
message. The same duplication the seam errors in this stack have been
shedding.
`StoreCause::new` already carries any error whole, which is what
`store_id` does at each of its own call sites. A closure cannot be generic
over the error type, and the store fails in two of them — a lock and a
write — so this is a pair of free functions instead.
`prefixes_by_dir` is keyed by `FileSystem::canonicalize`, which for the host is `dunce::canonicalize` and strips the `\\?\` verbatim prefix. `Pruned::store_key` looked its directory up with `canonicalize_utf8()` — `std::fs::canonicalize`, which keeps that prefix. On Windows the two spellings never match, so the lookup always missed: `drops` answered false for every `<path>:<canister>` reference, and a bundle kept references to dependency canisters the selected environment leaves out. The extracted bundle rejects those at load, which is the failure this pruning exists to prevent. Both sides go through the seam now, so they spell a directory the same way by construction. `drops` is async in consequence, and the `retain` passes it fed cannot await, so `prune_environment` asks about every name the environment mentions up front and the passes became lookups. `mentioned_canisters` gathers those names and sits next to it: a name it fails to gather is a reference the bundle would keep.
`operations::build` made its build directory with `camino_tempfile` — a directory on this machine — handed the path to the build step, and then asked `files` whether the module was there and to read it back. The prebuilt step writes through `files` too, so today the two happen to agree; anything backing the seam with something other than this machine's filesystem would never see what the step wrote, and every build would end in `MissingWasmOutput`. `FileSystem::scratch_dir` hands out the directory instead, so the place the step writes to and the place the operation reads from are the same implementation's. The host's is a `tempfile` directory as before, removed when the returned `Scratch` drops. A script step still writes with the machine's own hands; that is the subprocess runner's host-shape, and it goes when the runner does.
`a_literal_component_needs_no_listing` described `glob::glob`'s optimization, not this code: every component is matched by listing its parent, which is what let a `..` slip through unmatched. The name now says what the test asserts, and it also asserts the other half — a literal component that names nothing yields nothing.
Consolidation reads every manifest through `FileSystem`, but six of the checks guarding those reads still called `Utf8Path::is_file`. A project whose files are not this machine's would report each canister, dependency, network and environment it names as missing, and never reach the read that would have found it. The glob branch beside the first of them already asked `files`; the explicit-path branch next to it did not. The two that asked `exists()` first now ask only `is_file`: a path that is a file exists.
`is_dir` answers through symlinks, so a directory that links back to an ancestor is one directory reachable under endlessly many paths, and `**` descended every one of them. On macOS the walk stopped only when the kernel refused a seventeenth link, having matched the same file sixteen times under ever longer names; an implementation with no such limit would not have stopped at all. Descending is keyed on `canonicalize` now, so the first spelling of a directory is the one that matches and a later one matches nothing — the files under it are the same files, and reporting them twice would make one canister look like two. An implementation that cannot establish identity is descended anyway: it has no links to come back around, or it would be able to resolve them.
The feature listed subprocesses among what it gates, which it does not: build script steps go straight to `tokio::process`, and `HostScripts` sits in front of the sync ones without a gate. Randomness and the `tar` directory walk are outside it too, and every dependency is still linked either way, so the list now says what is covered and names what is not. `ArchiveWriter::dir` gets the reason it reads the host filesystem rather than the seam the rest of bundling reads through: `tar`'s own walk is what keeps a symlink a symlink, and the seam reports a link as the file it points at with no way to say otherwise.
adamspofford-dfinity
force-pushed
the
spofford/abstract-filesystem
branch
from
September 14, 2026 19:14
059afe5 to
93758ee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack created with GitHub Stacks CLI • Give Feedback 💬