refactor: Abstract out plugin running from icp-project - #775
Open
adamspofford-dfinity wants to merge 5 commits into
Open
adamspofford-dfinity wants to merge 5 commits into
adamspofford-dfinity wants to merge 5 commits into
Conversation
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.
adamspofford-dfinity
force-pushed
the
spofford/abstract-plugin-runner
branch
from
September 14, 2026 14:23
e654f6e to
c539772
Compare
adamspofford-dfinity
force-pushed
the
spofford/abstract-plugin-runner
branch
from
September 14, 2026 16:45
e457704 to
6d09e8a
Compare
adamspofford-dfinity
force-pushed
the
spofford/abstract-plugin-runner
branch
from
September 14, 2026 18:34
6d09e8a to
c93dd89
Compare
…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`.
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.
The script seam next to it is verified through a recording runner; the plugin one was only ever reached end-to-end, so nothing pinned what the invocation is assembled from — the fetched module, the keys the declared paths were written under, the table the names resolve against — or that a runner's own failure is passed through rather than restated.
The mappings that replaced the inlined agent and proxy handling had no test of their own: update against query, the authority a `direct` request is made under, the cycles it carries, the target a name resolves to, and the reason a failed call gives the plugin. The imports are served from plain host state, so they are driven directly rather than through a component.
adamspofford-dfinity
force-pushed
the
spofford/abstract-plugin-runner
branch
from
September 14, 2026 19:14
c93dd89 to
ae13f51
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 💬