refactor: report progress through a new icp-events crate instead of the terminal

[raymondk]

Why

Progress reporting is what welded crates/icp-cli/src/operations/ to the binary: six of those files imported crate::progress, a seventh drove indicatif directly, and two crates/icp ports took a tokio::sync::mpsc::Sender<String> that only existed because the binary had a progress bar to feed. Nothing else in the crate split can start until that is inverted, so it goes first — and it lands as one change rather than a partial one, so the tree is never half-converted.

What

New crate crates/icp-events — progress and user-facing notices as data. Depends on serde and futures only; not on icp, not on an async runtime, not on anything terminal-shaped (indicatif, dialoguer, clap, console).

• Event — TaskStarted/TaskMessage/TaskPosition/TaskFinished, StepStarted/StepOutput/StepFinished, and Notice { level, message }
• TaskKind — closed enum carrying the three shapes that exist: Spinner, Steps { output_label }, Bytes { total }
• Reporter / Task — Task::run(fut, success, error) replaces ProgressManager::execute_with_progress. A dropped Task still emits TaskFinished { Neutral }, so an early return cannot leave a sink holding a bar that spins forever.
• OutputWriter — a cheap, cloneable handle for the lines a running step produces. Each line becomes an Event::StepOutput and is kept in the task's step log (capped at MAX_RECORDED_LINES_PER_STEP), so an operation that fails can replay the failing step once the bars are down.
• EventSink, DiscardSink, RecordingSink, CancelToken

crates/icp-cli/src/events.rs — IndicatifSink, now the only place in the workspace that knows about indicatif, and where the styles live.

Every operation converted. install.rs, settings.rs, binding_env_vars.rs, candid_compat.rs, build.rs, sync.rs, snapshot_transfer.rs and bundle.rs take a &Reporter; none takes debug: bool. The commands that drew their own bars — deploy.rs, network/start.rs, network/update.rs, canister snapshot upload/download — go through the reporter too.

build_many/sync_many/create_bundle keep one bool, renamed all_step_output: replaying every step of a failure rather than only the failing one is the only thing debug still decided once bar-hiding moved into the reporter.

Both port signatures changed. Build::build and Synchronize::sync — and the script, prebuilt, wasm-resolve, plugin and script-runner paths beneath them — take Option<OutputWriter> instead of Option<Sender<String>>. crates/icp gains no terminal-shaped dependency; it depends on icp-events, which is serde + futures.

Two things fall out of that:

• MultiStepProgressBar's channel plus the background task that drained it are gone, and with them end_step().await, which existed only to wait for that drain. Recording is now synchronous.
• the sync plugin runtime reported lines with try_send on a bounded channel, so a noisy plugin silently dropped output when the renderer fell behind. It no longer can.

progress.rs is deleted. The compiler was the gate, as the architecture note asked: it was removed and cargo check -p icp-cli --all-targets had to come back clean.

The two macOS failures

events::rendering_equivalence failed on macos-15 only, with an extra bare " ~ " frame captured before the [backend] prefix appeared.

Not the rebase, and not a new defect: ProgressBar::enable_steady_tick spawns a thread whose first act is to draw a frame (TickerControl::run ticks before its first wait), so a prefix set after that call races that tick. Both renderers had that ordering, so both could produce the frame; Linux and Windows happened to win the race and macOS did not. Reproduced deterministically by inserting a sleep into the pre-inversion path, which produced the exact failing value:

[" ~ ", "[backend] ~ ", "[backend]   Skipped (not an upgrade)"]

Fixed in the sink rather than in the assertion: every bar is now fully styled and labelled before it is added to the MultiProgress and, for spinners, before the ticker starts, so no frame can be drawn unlabelled. tests::a_spinner_is_labelled_before_its_first_tick waits out several tick intervals and asserts every raw frame carries the prefix.

Terminal output is unchanged — how that was verified

The previous revision compared old and new renderers frame-by-frame against a recording indicatif::TermLike. Those renderers no longer exist, so before deleting them their output was captured and pinned as literals in events.rs::rendering:

• spinner success and failure — full three-frame sequences, including the final ✔/✘
• the candid_compat skip, which used finish_with_message and drew exactly one frame — so Outcome::Neutral must call finish_with_message too, not set_message + finish, or it inserts an extra redraw
• multi-canister bar ordering
• the byte bar's at-rest line, character for character

Four things are normalized away, each because it is a property of the machine rather than of what was drawn: the blank padding row indicatif writes to fill the terminal line (a function of the frame it follows); the timer-driven animation glyph; SGR colour codes (a TermLike draw target is never stderr, so indicatif leaves the styled fields following console's stdout-based detection — the frames carry colour when cargo test runs from a terminal and not when it is piped); and a spinner frame with no message yet, which is the animation thread getting a frame in before the operation has said anything. The last is what the macOS failure was made of; the underlying missing-prefix defect is fixed, not hidden. normalization_strips_the_colour_codes_a_terminal_would_add covers the colour rule directly rather than leaving it assumed.

Bar widths are deliberately not pinned once bytes have moved: {wide_bar} takes whatever width the rest of the line leaves over, which includes a transfer rate whose text is as long as the machine happened to be fast. Those tests pin the glyph set and the done/total counter instead.

One intentional difference, called out rather than smoothed over: the byte-transfer bars in canister snapshot upload/download are still drawn under --debug, matching today's behaviour, where the spinners are hidden. They are the only indication a long transfer is moving.

Testability

Every converted operation is unit-tested by running it for real against a RecordingSink and asserting on the resulting Vec<Event> — task order, exact messages, outcomes, step titles, step output. The three added in this pass:

• build.rs — a build's whole reported shape, a failing step's output and failure outcome, canister ordering
• sync.rs — the same three, driven through an injected ScriptRunner that reports lines instead of spawning a shell
• snapshot_transfer.rs — a resumed upload reports the offset it recovered before anything else, so the bar opens where the last attempt stopped rather than at zero

icp's own suite gains one: a build step's output arrives through the writer and is retained for replay, with no terminal involved.

Deliberately not in this PR

• The 29 user-facing tracing calls inside operations/ are still tracing calls. Event::Notice exists and the sink routes it, so converting them needs no redesign. (INFO-level tracing is product output in this CLI — logging.rs installs a UserLayer that prints Level::INFO to stderr unprefixed.)
• Two bespoke spinners still build their own indicatif bars: commands/canister/migrate_id.rs and commands/identity/link/web.rs. Neither ever went through progress.rs. Their look is genuinely different — no prefix, their own tick sets and colours, println through the bar, finish_and_clear — so converting them would mean either changing what the user sees or growing the event model to carry per-call-site styling for two call sites. Both are recorded in .claude/architecture.md with a grep to check the boundary holds.

Stability

The event model is intentionally not semver-stable: publish = false, 0.x in lockstep with icp-cli, all enums #[non_exhaustive], TaskKind closed rather than an open string. The event stream does not drive --json — --json keeps meaning the command's final result, and no flag or feature is added that would change that.

Checks

cargo build, cargo fmt --check, cargo clippy --workspace --all-targets and cargo test --workspace are clean, locally and through the validation pipeline (review, test, document, lint). The integration suites need the test network launcher fixture; the identity_link_hsm* tests additionally need SoftHSM2 and the canister_snapshot_*_resume tests need mitmproxy. One integration assertion moved with the code: the debug! line that echoes step output now has the tracing target icp::events rather than icp::progress.

Pipeline fixes folded in

Three findings the original change missed, fixed during validation:

• goldens-ansi-sensitive — the goldens above were captured with colour off and would have failed whenever the test binary's stdout was a terminal. RecordingTerm now strips SGR codes in both its normalized and raw views, with a test that feeds escaped frames through and asserts both.
• architecture-grep-false-hits — the invariant check documented in .claude/architecture.md matched events::indicatif_reporter too, so it listed 11 files while the prose claimed 3. Narrowed to grep -rl 'use indicatif'.
• sink-emit-repeated-lock — five arms of IndicatifSink::emit each repeated the same lock-and-look-up; collapsed into one with_bar helper, so "events for an unknown task are ignored" lives in one place.

Follow-up: --debug output says which canister printed each line

A bug found after the inversion landed on this branch, fixed here rather than in a separate PR. Under --debug the bars are hidden, so the debug! line the sink logs for each step-output event was the only record of a build's output — and it carried just the line, so canisters building in parallel interleaved into an unattributable stream. The bars have always shown the name in their prefix, so the two paths disagreed.

BarState now keeps the prefix it gave the bar and the log line reuses it, so [backend] compiling reads the way the bar does. The prefix is kept at construction rather than read back off the bar. Byte tasks label their bars undecorated and keep doing so here; they report no step output today, so this settles the shape rather than changing anything. A line with no label, or one arriving for a task that has already finished, is still logged — bare — rather than dropped for want of a prefix.

Verified on a real parallel build, because interleaving is the whole point and a single-canister check would pass while proving nothing: five canisters under --debug produced 20 output lines, every one attributed, none misattributed, with the canister changing between all 19 consecutive pairs. Sink-level tests cover the interleaved case, the unlabelled and unknown-task cases, and the byte-task shape; the interleaving test was mutation-checked against the bare debug! it replaced.

Updating the integration assertions turned up a trap worth noting: sync_tests had a negative assertion that no sync step ran ("DEBUG icp::events: syncing" must not appear), which the new prefix would have made vacuously true — green while testing nothing. It now matches "] syncing", a shape only the runtime log produces and which the --debug config dump does not.

The pipeline also fixed a flaw in the new test helper: it captured debug! lines through a thread-local subscriber, but tracing resolves and caches whether a callsite is enabled the first time any thread reaches it, so another test getting there first could cache "disabled" and leave the capture silent. It now installs one global subscriber per test binary and keys captured lines by thread id; .claude/testing.md records that constraint.

[Copilot]

Pull request overview

Introduces an event-based progress abstraction to decouple CLI operations from terminal rendering.

Changes:

• Adds icp-events with events, reporting, recording, and cancellation APIs.
• Adds an indicatif event sink with rendering-equivalence tests.
• Migrates four deploy operations to Reporter with event-sequence tests.

Reviewed changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
.claude/CLAUDE.md	Documents the new crate.
.claude/architecture.md	Documents progress architecture.
Cargo.toml	Registers icp-events.
Cargo.lock	Locks the new crate.
crates/icp-events/Cargo.toml	Configures crate dependencies.
crates/icp-events/src/cancel.rs	Implements cancellation tokens.
crates/icp-events/src/event.rs	Defines the event model.
crates/icp-events/src/lib.rs	Exposes the public API.
crates/icp-events/src/reporter.rs	Implements reporters and tasks.
crates/icp-events/src/sink.rs	Implements event sinks.
crates/icp-cli/Cargo.toml	Adds the crate dependency.
crates/icp-cli/src/commands/deploy.rs	Supplies reporters to operations.
crates/icp-cli/src/events.rs	Renders events with indicatif.
crates/icp-cli/src/main.rs	Registers the events module.
crates/icp-cli/src/operations/binding_env_vars.rs	Migrates environment-variable progress.
crates/icp-cli/src/operations/candid_compat.rs	Migrates compatibility-check progress.
crates/icp-cli/src/operations/install.rs	Migrates installation progress.
crates/icp-cli/src/operations/mod.rs	Registers test support.
crates/icp-cli/src/operations/settings.rs	Migrates settings progress.
crates/icp-cli/src/operations/snapshot_transfer.rs	Reuses byte-bar styling.
crates/icp-cli/src/operations/test_support.rs	Adds shared operation fixtures.
crates/icp-cli/src/progress.rs	Exposes shared progress styles.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[raymondk]

Review focused on test redundancy and comment noise. The design is sound and everything passes locally (cargo test -p icp-events -p icp-cli --bins: 40 + 105 pass; cargo clippy --all-targets clean). Inline comments cover the file-specific items; the cross-cutting ones are below.

1. A large part of the new crate has no callers

This is the biggest "unnecessary" item. Grepping the workspace outside crates/icp-events/src:

API	Callers
cancel.rs — CancelToken, Cancelled, run_until, Reporter::with_cancel_token, cancel_token()	none
Event::Notice + NoticeLevel + Reporter::notice/info/warn/error	none — nothing constructs a Notice; the sink arm is an admitted placeholder
Reporter::discard() / DiscardSink	none
Task::step_output	none (call sites all use task.output().line())
RecordingSink::take	none
MAX_RECORDED_LINES_PER_STEP as pub	internal only

cancel.rs is the costly one: 332 lines including a keyed waker registry with drop-time deregistration, plus 10 tests, none of which any operation exercises. Its subtleties (re-poll waker replacement, re-check under lock) are exactly the kind of thing that rots when unexercised.

My suggestion is to cut cancel.rs and the Notice/discard surface from this increment and land each with the change that needs it. If you'd rather keep the model complete up front, that's a defensible call — but then it's worth saying so in the PR body, because as it stands a reviewer can't tell "planned" from "orphaned".

2. Six copies of the same ordering test

operations/build.rs, sync.rs, install.rs, candid_compat.rs, binding_env_vars.rs, settings.rs all assert task_labels(...) == [frontend, backend], each ~25 lines of call-site boilerplate. Plus events.rs's bars_are_drawn_in_the_order_the_canisters_were_given and reporter.rs's task_ids_are_handed_out_in_creation_order for the same property one layer down. The invariant is per-call-site so it isn't pure duplication, but eight tests for "don't move reporter.task() inside the async block" is a lot — two (one steps-op, one spinner-op) would cover the regression at a fraction of the size. Details inline on operations/build.rs.

3. The same fact explained three times

Two subtle facts each get explained in three places, which means three places to update when either changes:

• ANSI/colour detection: strip_ansi's doc, RecordingTerm::frames' 15-line doc, and normalization_strips_the_colour_codes_a_terminal_would_add's doc.
• The enable_steady_tick race: IndicatifSink::start, a_spinner_is_labelled_before_its_first_tick, and .claude/architecture.md.

One site should carry each explanation; the others can point at it.

Separately, this comment is duplicated verbatim in five files:

// Started up front so the tasks appear in the order the canisters were given, regardless of the order the futures below are first polled in.

It's a good comment — it just doesn't need to be five comments. One in test_support.rs or the architecture doc, referenced from the call sites, would do.

What's good

The frame-golden approach in mod rendering is the right call for a refactor that deletes its own baseline; capturing the literal pre-inversion output is far more convincing than eyeballing it. step_replay.rs and its tests are tight — five tests, no overlap, each pinning one formatting rule. The Task drop-closes-neutrally guarantee is the correct invariant and is tested at the right level. And the --debug canister-prefix fix is a genuine user-visible improvement that the CHANGELOG entry describes plainly.

[raymondk]

Nothing outside this crate uses CancelToken, Cancelled, run_until, Reporter::with_cancel_token, or cancel_token() — the module's own tests are its only consumers.

That's 332 lines including a keyed waker registry with drop-time deregistration, plus 10 tests, for machinery no operation exercises. The subtle parts (waker replacement on re-poll, the re-check under the lock) are exactly what rots when nothing calls them.

I'd land this with the increment that actually needs cancellation.

[raymondk]

Reporter::discard() and DiscardSink have no callers outside the crate. The two tests that cover them (a_discarding_reporter_still_works here, discard_sink_keeps_nothing in sink.rs) are both assertion-free.

[raymondk]

notice/info/warn/error — and Event::Notice / NoticeLevel behind them — have no callers. Nothing in the workspace constructs a Notice; events.rs has a receiving arm that the comment there describes as a placeholder for a separate work item.

Five methods, an enum, and two tests for a path that doesn't exist yet. Worth landing with the conversion that uses it.

[raymondk]

step_output has no callers outside this crate's tests — every real call site goes through task.output().line(). It's a one-line convenience wrapper; probably fine to drop until something wants it.

[raymondk]

This test, and sinks_are_usable_from_several_threads in sink.rs, assert Send/Sync and that Mutex works. Both are compile-time properties — a fn _assert_send<T: Send>() {} plus a call site says the same thing without spawning threads, and fails at compile time rather than at test time.

[raymondk]

A second test module next to the existing mod tests — the new tests could just go in tests. It's the only file in the PR that splits them.

[raymondk]

task_labels is imported inside the function body here, while every sibling test module imports it at the top with the rest of test_support. Worth making consistent.

[raymondk]

Resolved by deletion rather than by moving the import: this is one of the six near-identical tasks_are_started_in_the_order_the_canisters_were_given tests, and per your other comment I kept two — build.rs (a steps operation) and install.rs (a spinner operation) — and dropped the rest, this one included.

So there is no longer an inconsistent inline import here. The two survivors both import from test_support at the top of their module with everything else.

[raymondk]

These two adjacent arguments are both derived from ctx.debug but mean opposite things — hide the bars vs. replay more output on failure. Same shape in deploy.rs and commands/sync.rs.

The rename to all_step_output helps at the definition; the call site still reads as ctx.debug twice, which is easy to get wrong when someone edits this later.

[raymondk]

This comment (and the matching one on l.131) restates the line below it — let stdio = stdio.clone(); already says it clones the writer for the task.

Pre-existing, but the PR touches these exact lines, so it's a free cleanup.

[raymondk]

// Report the frontier position restates task.position(progress.blob_progress(blob_type).frontier) directly below it. The companion comment on the initial position ("a resumed download starts partway in") earns its place because it explains why; this one doesn't add anything.