refactor: Abstract out plugin running from icp-project - #775
Open
adamspofford-dfinity wants to merge 3 commits into
Open
refactor: Abstract out plugin running from icp-project#775adamspofford-dfinity wants to merge 3 commits into
adamspofford-dfinity wants to merge 3 commits into
Conversation
…ound `icp-sync-plugin` named a concrete `ic_agent::Agent`, and `icp-project` depended on `icp-sync-plugin` to run a plugin step. Both are now the other way round: the runner is a seam `icp-project` declares, `icp-sync-plugin` implements it, and it reaches canisters through `calls::CanisterCalls`. So nothing on the sync path names an agent any more, and `ic-agent` (and `ic-utils`, and wasmtime through this crate) leave `icp-project` entirely. The split falls where the WIT interface already implied it should. Deciding what a step declared, what a canister name resolves to and what a plugin may call is manifest work, so `KeyedPath`, `CallableCanisters` and the invocation itself move to `icp-project`, defined once and consumed by the runner — `deploylib` restated these in two crates and hand-copied sixteen fields between them. What stays behind is what needs a machine: a component runtime, a WASI sandbox, a compute deadline, a filesystem to open the declared paths on. `Synchronize::sync` loses its `&Agent`, and so do `sync_many` and `deploy`. `covering_dirs` and `distinct_paths` come along, because bundling uses them too and a manifest's declared path set is not a runtime concept. The rest of `path.rs` — the sandbox resolution, which reads symlinks — stays put. The plugin's two `direct` flags are what made this more than a move. `direct` asks for a call to skip the proxy and be made by the sync identity itself, which is a choice between two authorities the caller has, so `Authority` (`Mediated`/`Direct`) says which one a request wants and the implementation decides what that takes. `AgentCalls` therefore absorbs the last of the proxy knowledge the runtime was carrying: encoding a proxied call, and the whole two-route metadata read — a certificate for a direct read, a management `canister_metadata` call the proxy makes for a mediated one, with the rule that only a certificate can report a section absent. That is ~110 lines of hash-tree and reject-text handling out of the plugin runtime and into the crate that owns the transport, and it is what "the abstraction is simply assumed to return certified answers" has to mean. Two consequences worth naming: - A query now always goes direct, whatever the plugin asked. It already did before this change; saying so explicitly matters because `CanisterCalls` turns a mediated query into an update, and the interface documents queries as reaching the target itself — a plugin would otherwise have silently paid cycles for one. - `metadata_section` is now the certificate the plugin runtime used, not the two round trips `AgentCalls` was doing. A certificate that proves nothing about the section is an error rather than being read as absence when `controllers` happens to be present, which was already the plugin's rule and is the safe direction. `fetch_canister_metadata` reads under `Authority::Direct`, which is what it did before: a capability probe asks what the deploying identity can see. Verified: 388 unit tests, and 119 integration tests across sync (17, including `sync_plugin_routes_through_proxy`, which exercises the proxied metadata read end to end), bundle (29), deploy (25), install (20), create (17), delete (4) and call (7). `cargo check -p icp-project --no-default-features` is clean, and building `icp-sync-plugin` alone now compiles `icp-project` without `host` — which caught four host-only imports that were not gated.
`RunError` 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 other seam errors: `network::Access`, `canister::wasm::Fetch`,
`canister::recipe::Resolve` and `files::FileSystem`.
adamspofford-dfinity
added this pull request to stack #777
September 11, 2026 15:05
Both host imports flattened a `CallError` with `to_string()`, and that string is what the guest receives and what surfaces as the sync step's error. `CallError::Failed`'s own message names only the call — the cause is boxed and meant to be walked — so an unreachable replica or a reply that fails to candid-decode reached the plugin as `call to 'store' on <id> failed` and nothing else. `Rejected` interpolates its message, so it was only the failure class where the detail matters most that lost it. `error::flatten` renders an error and its `source()` chain as one string, for exactly this: a boundary that takes a single message. It sits next to `error::causes`, which is `sync_many`'s chain walk for the failed-task event moved up out of `operations::sync`, and it replaces the same walk written inline in the compute-limit test.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Core runner dispatch and canister-call routing introduced by the refactor lack regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Abstracts sync-plugin execution behind icp-project traits while keeping Wasmtime and agent-specific behavior in host crates.
Changes:
- Adds injectable plugin-running and canister-call seams.
- Moves shared path and error utilities into
icp-project. - Rewires sync/deploy flows to use the new abstractions.
File summaries
| File | Description |
|---|---|
.claude/CLAUDE.md |
Documents crate boundaries and seams. |
Cargo.lock |
Updates dependency graph. |
crates/icp-app/Cargo.toml |
Adds plugin-runtime dependency. |
crates/icp-app/src/calls.rs |
Implements metadata authority and proxy routing. |
crates/icp-app/src/context/init.rs |
Injects the Wasmtime runner. |
crates/icp-cli/src/commands/deploy.rs |
Removes separate agent forwarding. |
crates/icp-cli/src/commands/sync.rs |
Passes the call abstraction into sync. |
crates/icp-project/Cargo.toml |
Removes runtime-specific dependencies. |
crates/icp-project/src/calls.rs |
Extends call abstractions and test mocks. |
crates/icp-project/src/canister/sync/declared.rs |
Hosts declared-path reduction utilities. |
crates/icp-project/src/canister/sync/mod.rs |
Injects plugin runners into Syncer. |
crates/icp-project/src/canister/sync/plugin.rs |
Defines and invokes the plugin-runner seam. |
crates/icp-project/src/error.rs |
Adds error-chain rendering utilities. |
crates/icp-project/src/lib.rs |
Exposes error utilities. |
crates/icp-project/src/operations/bundle.rs |
Uses relocated path helpers. |
crates/icp-project/src/operations/deploy.rs |
Uses shared canister calls for sync. |
crates/icp-project/src/operations/misc.rs |
Selects direct metadata authority. |
crates/icp-project/src/operations/sync.rs |
Passes call abstractions through synchronization. |
crates/icp-sync-plugin/Cargo.toml |
Replaces agent dependencies with project seams. |
crates/icp-sync-plugin/DESIGN.md |
Updates runtime architecture documentation. |
crates/icp-sync-plugin/src/lib.rs |
Exposes only the Wasmtime implementation. |
crates/icp-sync-plugin/src/path.rs |
Removes relocated path-reduction logic. |
crates/icp-sync-plugin/src/runtime.rs |
Implements plugin execution through shared seams. |
Review details
- Files reviewed: 22/23 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+255
to
+260
| // 4. Hand it all to the runner. | ||
| plugins | ||
| .run(Invocation { | ||
| wasm_path, | ||
| base_dir, | ||
| project_dir, | ||
| base_dir: params.path.clone(), | ||
| project_dir: params.project_dir.clone(), |
Comment on lines
+136
to
+142
| let mut call = Call::new(target, method, arg_bytes).with_cycles(cycles.into()); | ||
| // A query goes to the target itself whichever way the plugin asked, | ||
| // which is what the interface documents: an intermediary that only | ||
| // accepts updates would otherwise turn one into an update, and the | ||
| // plugin would have paid for it. | ||
| if direct || query { | ||
| call = call.direct(); |
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 💬