Skip to content

refactor: Remove Context from operations - #770

Open
adamspofford-dfinity wants to merge 4 commits into
mainfrom
spofford/de-context-operations
Open

refactor: Remove Context from operations#770
adamspofford-dfinity wants to merge 4 commits into
mainfrom
spofford/de-context-operations

Conversation

@adamspofford-dfinity

@adamspofford-dfinity adamspofford-dfinity commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Stack created with GitHub Stacks CLI • Give Feedback 💬

@adamspofford-dfinity
refactor: give operations a project-scoped Host instead of Context
8d42d22 
`operations::deploy` and `operations::settings` were the only things in
`icp` reaching for `Context`, and `Context` is the bag that also holds the
identity loader, the keyring, the global directories and the password
prompt. Nothing app-scoped can leave this crate while an operation can
still reach all of that.

`icp::host::Host` is that surface instead: the project loader, the id and
artifact stores, the builder, the syncer, network resolution, and the
environment/canister-id resolution methods that used to hang off
`Context`. `Context` now holds a `Host` beside its own app-side fields.
`deploy` takes `(&Host, &Agent, &PackageCache, ...)` — the agent and the
package cache resolved by the command — so the operations layer no longer
knows that identities exist.

`Context::update_custom_domains` becomes
`network::Access::publish_friendly_domains`. The project side collects
every environment's `friendly name -> canister id` entries; the network
side decides which of them a running gateway serves, and where the file
goes. That was the one project-to-app call inside `deploy`.

Two intended behavior changes fall out:

- The identity is unlocked before the build rather than after, so a deploy
  aimed at a network that is not running fails before spending time on a
  build.
- `icp deploy` builds one agent for the whole run and reuses it for the
  URLs printed at the end rather than building a second one, so a network
  with `root-key: fetch` is fetched, and warned about, once.

To keep the first of those from costing an error message, `resolve_targets`
now checks the names it was given while that is still a question about the
project alone: a canister-name typo is reported as such instead of being
pre-empted by whatever an unreachable network had to say.
@adamspofford-dfinity
refactor: resolve the deploy agent lazily, collect domains on demand
067b7bf 
Follow-ups to the Host extraction:

- `deploy` takes a `LazyAgent` and resolves it only once the build has
  succeeded. Resolving it up front meant a deploy with a broken build
  unlocked the identity's key — a password prompt, for an encrypted one —
  and, on a stopped managed network, reported the network error in place
  of the build error.

- `Access::publish_friendly_domains` is handed the means to collect the
  project's friendly-name mappings rather than a finished collection, and
  asks only once it knows there is something to write and which network
  is being served. A `canister create`/`delete` against a stopped network
  no longer does an id-store read per environment for nothing.

- That method is now required rather than defaulting to a no-op: a second
  implementor, or a decorator over `Access`, would otherwise silently
  stop publishing friendly domains with no compile error.

- `Context::telemetry_data` is gone in favour of the `Host`'s. The two
  were separate fields the design assumed aliased one `Arc`, which the
  `Context { host: Host { .. }, ..Context::mocked() }` pattern in the
  tests quietly breaks.
@adamspofford-dfinity
fix: stop appending the validation cause to its own message
3f63e66 
`CanisterMigrationError::ValidationFailed` interpolated `{source}` into its
display while also reporting it as a source, so the cause printed twice in
every chain. Its three sibling variants already left the cause to the chain;
this one was the exception.

Predates this stack; it is fixed at the base so the rest of the stack carries
the fix.
@adamspofford-dfinity
fix: stop appending causes to the messages that wrap them
8b0f5cf 
Twenty-three error variants interpolated `{source}` into their own display
while also reporting it as a source, so every chain printed the cause twice —
once inside the wrapping message and once beneath it:

    Error: failed to get available subnets: canister rejected the call

    Caused by:
        canister rejected the call

Each message keeps the context it adds, naming the operation that failed, and
the cause is left to the chain that already reports it. Nothing is lost: these
all reach the user through the anyhow chain, which prints every source below
the top line.

Predate this stack, so they are fixed at its base and the rest of the stack
carries the fix.
Copilot AI balanced review requested due to automatic review settings September 11, 2026 15:05
@adamspofford-dfinity
adamspofford-dfinity requested a review from a team as a code owner September 11, 2026 15:05
@adamspofford-dfinity
adamspofford-dfinity added this pull request to stack #777 September 11, 2026 15:05
Copilot AI reviewed Sep 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Agent creation currently precedes the local --no-create validation, causing unnecessary prompts or network failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Refactors operations to use a project-scoped Host instead of the broader CLI Context.

Changes:

  • Introduces Host and migrates project, environment, network, build, and ID-store access.
  • Adds lazy agent creation for deploys and relocates friendly-domain publishing.
  • Simplifies operation error displays while preserving error sources.
File summaries
File Description
crates/icp/src/operations/token/balance.rs Simplifies parse error display.
crates/icp/src/operations/token/approve.rs Simplifies parse error display.
crates/icp/src/operations/token/allowance.rs Simplifies parse error display.
crates/icp/src/operations/settings.rs Migrates settings operations to Host.
crates/icp/src/operations/proxy.rs Simplifies proxy error displays.
crates/icp/src/operations/proxy_management.rs Simplifies log-fetch error displays.
crates/icp/src/operations/deploy.rs Migrates deployment to Host and lazy agents.
crates/icp/src/operations/create.rs Simplifies creation error displays.
crates/icp/src/operations/canister_migration.rs Simplifies validation error display.
crates/icp/src/network/mod.rs Adds friendly-domain publishing abstraction.
crates/icp/src/lib.rs Exposes the new host module.
crates/icp/src/host.rs Defines project-scoped operation resources.
crates/icp/src/context/tests.rs Updates context tests for nested Host.
crates/icp/src/context/mod.rs Removes project resources from Context.
crates/icp/src/context/init.rs Initializes the new Host.
crates/icp/src/agent.rs Adds lazy agent creation.
crates/icp-cli/src/options.rs Updates selection imports.
crates/icp-cli/src/main.rs Reads telemetry through Host.
crates/icp-cli/src/complete.rs Loads projects through Host.
crates/icp-cli/src/commands/sync.rs Migrates sync command dependencies.
crates/icp-cli/src/commands/project/show.rs Loads projects through Host.
crates/icp-cli/src/commands/project/bundle.rs Migrates bundle resources.
crates/icp-cli/src/commands/network/stop.rs Migrates network access.
crates/icp-cli/src/commands/network/status.rs Migrates network access.
crates/icp-cli/src/commands/network/start.rs Migrates network and ID access.
crates/icp-cli/src/commands/network/ping.rs Migrates network access.
crates/icp-cli/src/commands/network/list.rs Loads projects through Host.
crates/icp-cli/src/commands/environment/list.rs Loads projects through Host.
crates/icp-cli/src/commands/deploy.rs Constructs and reuses a lazy agent.
crates/icp-cli/src/commands/canister/status.rs Migrates environment access.
crates/icp-cli/src/commands/canister/settings/update.rs Migrates project access.
crates/icp-cli/src/commands/canister/settings/sync.rs Migrates canister and ID access.
crates/icp-cli/src/commands/canister/migrate_id.rs Updates selection import.
crates/icp-cli/src/commands/canister/list.rs Migrates environment access.
crates/icp-cli/src/commands/canister/link.rs Migrates ID-store operations.
crates/icp-cli/src/commands/canister/install.rs Migrates artifact access.
crates/icp-cli/src/commands/canister/delete.rs Migrates ID and domain updates.
crates/icp-cli/src/commands/canister/create.rs Migrates creation dependencies.
crates/icp-cli/src/commands/canister/call.rs Migrates environment and artifact access.
crates/icp-cli/src/commands/build.rs Migrates build resources.
crates/icp-cli/src/commands/args.rs Updates selection imports.
Review details
  • Files reviewed: 41/41 changed files
  • Comments generated: 1
  • 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 thread crates/icp/src/operations/deploy.rs

// Everything from here on talks to the network, so this is where the agent
// gets made — and where the identity it speaks for gets unlocked.
let agent = agent.get().await?;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adamspofford-dfinity