ci: track stable in rust-toolchain.toml, enforce MSRV in its own job - #741
Merged
Conversation
rust-toolchain.toml was pinned to the MSRV (1.88.0), so every local build, rust-analyzer, clippy and every CI job used a compiler nine releases behind stable. That makes the repo progressively harder to work in as the MSRV ages (rust-analyzer now refuses sufficiently old toolchains) and hides new rustc/clippy diagnostics until the MSRV is bumped. Split the two concerns, matching the setup in dfinity/cdk-rs: - rust-toolchain.toml tracks `stable`. - `rust-version` in the workspace Cargo.toml stays at 1.88.0 and becomes the single source of truth for the MSRV. - A new `msrv` job in test.yml parses `rust-version` out of Cargo.toml and builds with that toolchain, so bumping the MSRV is a one-line change and cannot drift out of sync with CI. The msrv job covers the published crates only (ref-tests and ic-utils-bindgen-tests depend on pocket-ic from the IC monorepo, whose MSRV runs far ahead of ours) and builds without --all-targets: the promise is that consumers can build the libraries at the MSRV, not that our test suite runs there. It gates on the existing `changes` filter and reports through the existing `test:required` aggregate, so no branch-protection changes are needed. Verified before landing: cargo fmt, `cargo hack clippy --each-feature`, `clippy --all-targets --all-features -D warnings`, the wasm clippy target and `RUSTDOCFLAGS=-Dwarnings cargo doc` all pass on stable 1.97, and the new msrv commands pass on 1.88.0. No source changes were needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…doctests Moving rust-toolchain.toml to `stable` surfaced two latent problems in the WASM job. Both are test-harness issues; no library code changes. 1. `agent::agent_test::no_cert` failed with "some mocked routes were never hit". The service-worker mock counts hits with a read-modify-write across two IndexedDB transactions: getMock reads the whole record, the handler mutates its copy, setMock writes it back. `Agent::query` issues its `query` and its `read_state` concurrently via `try_join!`, so both handlers read the pre-state and the second write discards the first one's increment. `no_cert` is the only test that combines concurrent requests with assert_mock (which requires *every* route to be hit), so it is the only one that noticed. Confirmed rather than inferred: the counter for the `query` route reads 0 even though the test's MissingSignature assertion — which requires the query response to have been served — passes. The request happened; only its increment was lost. Fixed by serializing the service worker's handlers; the responses are canned data, so there is nothing to gain from overlapping them. Locally: fails 2/2 on stable 1.97 and passes 2/2 on 1.88 before the fix, passes 3/3 on both after. assert_mock now prints the hit map on failure. The original assertion gave no indication of which route was missed. 2. Newer toolchains run doctests for wasm targets, which 1.88 skipped. ic-agent's doctests use `#[tokio::main]` and tokio is deliberately a dev-dependency only under cfg(not(target_family = "wasm")), so they cannot compile there: "cannot find module or crate `tokio`". Pass --lib so the browser run covers the #[wasm_bindgen_test] tests it is meant to cover; the host `test` job already runs the doctests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lwshang
marked this pull request as ready for review
August 5, 2026 13:17
|
✅ No security or compliance issues detected. Reviewed everything up to d295c59. Security Overview
Detected Code Changes
|
adamspofford-dfinity
approved these changes
Aug 5, 2026
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
2 participants
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.
Description
rust-toolchain.tomlwas pinned to the MSRV (1.88.0), so local builds, rust-analyzer andevery CI job ran on a compiler nine releases behind stable — hence the "toolchain too old"
warning rust-analyzer now emits.
This decouples the two, matching dfinity/cdk-rs:
rust-toolchain.tomlstablerust-versionin workspaceCargo.toml1.88.0(unchanged)The MSRV itself does not change. A new
msrvjob intest.ymlenforces it, building thepublished crates with the version it reads out of
Cargo.toml— so an MSRV bump stays aone-line edit and can't drift from CI. It reports through the existing
test:requiredaggregate, so no branch-protection changes are needed.
The newer compiler also surfaced two latent bugs in the WASM job, both test-harness only
(d295c59): the mock service worker lost hit-counter increments when a test had two requests
in flight, and doctests now run against wasm, where ic-agent's
#[tokio::main]examplescan't compile (
--libskips them; the hosttestjob still runs them).How Has This Been Tested?
CI is green. Every gate that now floats with the toolchain was also run locally on stable
1.97 (fmt, clippy incl.
cargo hack --each-feature, wasm clippy, rustdoc) and the MSRVbuilds on 1.88.0. The WASM fix was verified repeatedly on both toolchains.
Notes for reviewers
msrvjob is Linux-only — MSRV drift is nearly always a dependency raising its ownrust-version, which is platform-independent. Say so if you'd rather addwindows-2025.release.ymlnow cross-buildsicxwith floating stable instead of a frozen1.88.0.CARGO_TARGET_DIR=target/wasmresolves inside
ic-agent/(wasm-pack cds first), sorust-cachenever caches it.Checklist: