Skip to content

ci: workflow overhaul — path gating, benchmark signal, cargo-deny v2, naming convention - #757

Merged
lwshang merged 8 commits into
masterfrom
lwshang/ci-overhaul
Jul 28, 2026
Merged

ci: workflow overhaul — path gating, benchmark signal, cargo-deny v2, naming convention#757
lwshang merged 8 commits into
masterfrom
lwshang/ci-overhaul

Conversation

@lwshang

@lwshang lwshang commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

General cleanup of .github/workflows, adopting the Rust CI practices from icp-cli and cdk-rs.

On a documentation-only PR, rust, fuzzing, tools, bench and dependencies now all stop running, instead of all five running and one of them posting a table.

Changes

  • Path-gate every PR workflow.
  • Benchmarks: report to the job summary instead of a PR comment, and fail on a >10% instruction-count regression.
  • cargo-deny: upgrade to v2 and make the sources check enforceable.
  • Make the required rust check an aggregate job.
  • Shared Rust setup for every job that runs Rust — drop the image's bundled toolchain, then actions-rust-lang/setup-rust-toolchain for install and caching. Removes the hand-rolled actions/cache steps.
  • Lint rust/bench, which no lint gate reached.
  • Adopt a workflow naming convention, documented in .github/workflows/README.md.
  • Housekeeping: pin runners to ubuntu-24.04, add concurrency groups, install cargo-fuzz prebuilt rather than from source, cache canbench on its own version, bump the actions flagged by the Node 20 deprecation warning.

Behavior changes worth knowing

  • rust.yml is gated per job, not by paths:. A required check whose workflow never starts is never created, and branch protection then waits on it forever. A changes job decides instead. Non-required workflows use plain paths: filters.

  • The required rust check is now an aggregate. The build and test work moved to a check job; rust only needs: [check, fuzzing] and fails if either did. It runs under a bare if: always(), so it reports even when everything it watches was skipped — merging no longer depends on how GitHub scores a skipped job. Future work jobs gate by joining its needs:, with no branch protection change.

  • Benchmarks post to the job summary instead of commenting. The comment was unconditional while nothing ever failed on a regression, so it was output nobody had reason to read — and it broke on every fork PR (fix(candid): newtype elements and binary format in the primitive-vec fast path + release candid 0.10.34 #755 failed four times), since forks get a read-only token whatever permissions: says. Step summaries need no token, so forks behave identically.

  • A >10% instruction-count regression now fails the bench job. Green means no regression, red means open the summary. Only instruction counts are gated: they are deterministic under canbench, whereas heap_increase is page-granular and would flake on small footprints.

  • cargo-deny moved to v2. deny.toml used default, unlicensed and deny, all removed in cargo-deny 0.16, so bumping the action alone would have broken the config. Allow-list unchanged, no new findings.

  • An unexpected dependency source now fails. cargo-deny only warns about unknown git and registry sources by default, so check sources could never fail here. Both are now deny: a git revision is mutable and never reaches crates.io, so it cannot ship in a published release.

  • Four workflows renamed, following <subject>[-<qualifier>].yml with name: as its Title Case rendering. license.yml becomes dependencies.yml because it runs bans, licenses and sources — and because that is where an advisories check would later belong.
    bench.yml → rust-bench.yml Rust Bench
    license.yml → dependencies.yml Dependencies
    candid-ui.yml → candid-ui-release.yml Candid UI Release
    publish.yml → crates-publish.yml Crates Publish

  • No branch protection change is needed. The required context is still rust, and every renamed workflow is non-required. The :required suffix convention is dropped: GitHub already labels required checks, and the marker had drifted onto license-check, which is not required, while rust, which is, never carried it.

Notes for review

  • rust.yml produces four checks — detect changes, check, fuzzing, and the required rust aggregate. Only the last gates merging; the rest are informational.
  • The skip path cannot be exercised from this branch, since every PR from it includes rust.yml in the diff. The aggregate makes that moot for merging, but the filter's behaviour on an unrelated PR is unverified.
  • Linting rust/bench surfaced two real clippy errors, fixed here.
  • actionlint is clean on every workflow. Its only output is pre-existing shellcheck style noise in the generated didc-release.yml, which is written by dist and must not be hand-edited.
  • candid still has no RUSTSEC scanning — no advisories check, no dependabot.yml. Out of scope here, but worth a follow-up.

lwshang and others added 4 commits July 27, 2026 18:30
@lwshang @claude
ci: overhaul workflows — required-check gating, fork-safe bench, shar…
91c9c05 
…ed Rust setup

Path-gate the PR workflows so unrelated changes stop running the full
Rust suite, fix the benchmark job for pull requests from forks, and adopt
the Rust CI practices used in dfinity/cdk-rs.

Required-check gating
---------------------
`rust` is the only required status check on master, which rules out a
workflow-level `paths:` filter: a workflow that never starts produces no
check run, and branch protection then waits forever on a check that will
never arrive.

rust.yml therefore always starts, and a `changes` job decides whether the
work runs. Gated jobs are skipped, which GitHub reports as passing for
required checks. If detection itself fails the gate falls open and the
jobs run, so a broken filter cannot silently green-light the check.

tools.yml and bench.yml are not required, so they use plain
workflow-level `paths:` filters.

license.yml: renamed the job from `license-check:required` to
`license-check`. The `:required` suffix is this repo's marker for checks
in branch protection, and this one is no longer among them.

Benchmark on forks
------------------
Every fork pull request failed at the "Post comment" step with "Resource
not accessible by integration" (most recently #755). Fork pull requests
get a read-only GITHUB_TOKEN no matter what `permissions:` declares, so
the comment can never be posted from the same job.

Split the comment into bench-comment.yml, triggered on `workflow_run`,
which runs in the base repository context with a writable token. It
downloads a text artifact and posts it, and never executes pull request
code. The PR number travels in the artifact because
`github.event.workflow_run.pull_requests` is empty for fork PRs, and is
validated as digits-only before use.

bench.yml is now `permissions: contents: read` and path-filtered, so it
no longer runs — or comments — on documentation-only pull requests.

Shared Rust setup
-----------------
Adopted from dfinity/cdk-rs for every job that runs Rust: remove the
runner image's pre-installed stable toolchain (its version varies between
image releases and makes the rust-cache hash non-deterministic), then use
actions-rust-lang/setup-rust-toolchain, which installs the toolchain and
handles caching internally. Dropped the hand-rolled `actions/cache`
steps.

Also: cargo-fuzz now installs as a prebuilt binary via
taiki-e/install-action instead of compiling from source; canbench is
cached on its own version rather than the Cargo.lock hash; runners are
pinned to ubuntu-24.04; concurrency groups added to rust.yml and
tools.yml; and the actions flagged by the Node 20 deprecation warning
were bumped. All actions remain pinned to full commit SHAs.

Not touched: didc-release.yml is generated by cargo-dist (regenerate with
`dist generate`), and coq.yml is already path-gated and runs no Rust.

Verified with actionlint: no findings outside the generated
didc-release.yml.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang @claude
ci: trim workflow comments, lint the bench crate
2730dbd 
Comments now carry only what someone editing the workflow needs; the
rationale lives in the PR description.

rust/bench was outside every lint gate: it is its own workspace, so the
root `cargo fmt --all` and `cargo clippy` never reached bench.rs and
nns.rs, and its manifest had no `[lints]` table. Added fmt and clippy
steps to bench.yml, mirrored the root clippy policy into the bench
workspace, and fixed the two `manual_repeat_n` errors this surfaced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang @claude
ci: pin the fuzz build to the host target
03d9acc 
cargo-fuzz derives its default `--target` from its own build-time triple.
taiki-e/install-action fetches the upstream prebuilt binary, and the only
Linux asset published is x86_64-unknown-linux-musl, so the build was sent
to a musl target with no std installed and where sanitizers cannot link
against a static libc.

The previous `cargo install cargo-fuzz` compiled a gnu binary locally,
which is why this did not surface until the switch to the prebuilt.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang @claude
ci: report benchmarks in the job summary and fail on regression
da58dd4 
Replaces the PR comment with a job summary and adds a 10% regression
threshold, deleting bench-comment.yml and everything that supported it.

The comment was unconditional, so on most pull requests it was noise —
nothing failed on a regression, so the table was output nobody had a
reason to read. Inverting that: the job summary always carries the
numbers, and the job fails only when an instruction count regresses past
the threshold, which is when someone has a reason to open it.

This also removes the fork problem rather than routing around it. A job
summary needs no token, so it behaves identically for fork pull requests.
Gone with it: the workflow_run companion holding a writable token, the
artifact handoff, the cross-run download, and the untrusted PR-number
scrub.

Only instruction counts are gated. They are deterministic under canbench,
so a move past the threshold is a real change rather than noise. Memory
is still reported but not gated: heap_increase is page-granular, so a
one-page move on a small footprint is a large percentage meaning nothing.

diff.py takes the threshold as an optional third argument and exits 1
when breached. The workflow captures that, writes the summary, and only
then fails, so the numbers are readable on a failing run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang lwshang changed the title ci: overhaul workflows — path gating, fork-safe benchmarks, shared Rust setup ci: overhaul workflows — path gating, benchmark signal, shared Rust setup Jul 28, 2026
lwshang and others added 2 commits July 27, 2026 20:25
@lwshang @claude
ci: upgrade cargo-deny to v2 and make the sources check enforceable
361b7da 
The pinned action was cargo-deny-action v1.6.3, and deny.toml still used
v1 syntax. `default`, `unlicensed` and the explicit GPL `deny` list were
all removed in cargo-deny 0.16, so the config was a hard validation error
on anything current — bumping the action alone would have broken it.

Migrated to the shape icp-cli uses: action v2.0.15, `rm rust-toolchain.toml`
so cargo-deny does not install a toolchain and the wasm32 target just to
read the dependency graph, and `unused-allowed-license = "allow"`. The GPL
deny list is dropped because anything absent from `allow` is now denied.

The allow list itself is unchanged, and `bans ok, licenses ok, sources ok`
both before and after, so this is a config migration with no policy change.

Two things are deliberately not copied from icp-cli:

* Their broader allow list (BlueOak, CDLA, ISC, MPL-2.0, Unicode-3.0,
  OpenSSL) covers their dependency graph, not ours. Keeping candid's list
  minimal is what makes it evident that the migration found nothing new.

* Their `[sources] allow-git` exists because they depend on agent-rs.
  candid has no git dependency and should not gain one — a git revision is
  mutable and never reaches crates.io, so it cannot ship in a published
  release. Since cargo-deny only *warns* about unknown git and registry
  sources by default, `check sources` here could never fail; both are now
  set to deny. Verified by temporarily pointing logos at a git tag, which
  produces `error[source-not-allowed]` and `sources FAILED`.

Also rewrote the header comment. It linked to two archived dfinity-lab
repos and justified allowing MPL-2.0, which is not and should not be on
candid's list — it described icp-cli's policy, not this one. Replaced with
the actual rule: candid is a library, so allow only licenses that place no
obligation on downstream users beyond attribution.

The workflow now runs only when the dependency graph or the policy changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang @claude
ci: adopt a workflow naming convention
cd90fcd 
Half the workflow files could not be guessed from their `name:` and vice
versa: bench.yml was "Rust Bench", license.yml "License Check",
candid-ui.yml "Release candid_ui", publish.yml "Publish crates to
crates.io". Casing varied three ways, job ids mixed camelCase with kebab,
and coq.yml's job was called `build`, which says nothing in a list of a
dozen checks.

The rule is now one line: the filename is the identity and `name:` is its
Title Case rendering, as `<subject>[-<qualifier>].yml`. Subject first,
including for release and publish, so related workflows group together
and so the one file we cannot edit sets the pattern instead of being an
exception to it. Written down in .github/workflows/README.md.

  bench.yml      -> rust-bench.yml         Rust Bench
  license.yml    -> dependencies.yml       Dependencies
  candid-ui.yml  -> candid-ui-release.yml  Candid UI Release
  publish.yml    -> crates-publish.yml     Crates Publish

license.yml becomes dependencies.yml because it does not only check
licenses -- it runs bans, licenses and sources -- and because that is
where the missing advisories check would later belong.

Job names now come from the id unless the id is insufficient, since
GitHub falls back to the id and a `name:` repeating it is duplicated
state. Dropped from rust, fuzzing, cargo-deny, publish and candid-ui;
kept on `detect changes`. coq.yml's `build` is renamed `coq`, and
bench.yml's camelCase `runBenchMark` is renamed `bench`.

The `:required` suffix convention is dropped rather than corrected.
GitHub already labels required checks in the merge box, so the suffix
duplicated settings held elsewhere, and it had already drifted: it sat on
license-check, which is not required, while rust, which is, never had it.

No branch protection change is needed. rust.yml keeps both its filename
and its `name:`, so the required `rust` context and the README badge
(workflows/Rust/badge.svg) are untouched, and every renamed workflow is
non-required.

Also documents what testing dist established: didc-release.yml's filename
comes from `tag-namespace` and cannot be set independently of the release
tag prefix, and its `name: Release` is not configurable at all. Verified
that `dist generate` still reproduces the committed file byte for byte.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang lwshang changed the title ci: overhaul workflows — path gating, benchmark signal, shared Rust setup ci: workflow overhaul — path gating, benchmark signal, cargo-deny v2, naming convention Jul 28, 2026
lwshang and others added 2 commits July 27, 2026 20:56
@lwshang @claude
ci: make the required rust check an aggregate job
d194949 
`rust` was the job that built and tested; it is now an aggregate that
reports on the jobs above it, and the work moved to a `build` job. Same
shape as `test:required` in icp-cli's test.yml.

The reason to prefer it here is that the required check stops depending
on how GitHub scores a skipped job. Gating `rust` by a job rather than a
workflow-level `paths:` filter meant that on an unrelated pull request the
required check reported *skipped*, and merging relied on that counting as
success. The aggregate runs under a bare `if: always()`, so it executes
and exits 0 even when everything it watches was skipped. Nothing to
assume.

icp-cli's aggregate is additionally gated on its own paths filter, so it
skips too and still leans on that behaviour; ours deliberately does not.

It also means future work jobs gate merging by being added to `needs:`,
with no branch protection change — the configured context stays `rust`
either way, so no settings change is needed for this commit.

`skipped` counts as a pass, since it means the paths filter ruled the job
out. The aggregate does not gate on `changes`: the work jobs already fall
open when detection fails, so the work ran regardless, and a red
`detect changes` is the signal by itself.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang @claude
ci: rename the build job to check
4248ef2 
It runs cargo build, cargo test three ways, cargo fmt, cargo clippy and
cargo doc. `build` described the first step and nothing else — the same
problem as coq.yml's `build`, which this branch renamed two commits ago
for the same reason.

Kept as one job rather than split into build/test/fmt/clippy/doc the way
cdk-rs and icp-cli do. rust-cache keys per job, so splitting would
compile the workspace several times over for a job that currently takes
about a minute end to end. Worth revisiting if it grows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang
lwshang marked this pull request as ready for review July 28, 2026 01:15
@lwshang
lwshang requested a review from a team as a code owner July 28, 2026 01:15
@zeropath-ai

zeropath-ai Bot commented Jul 28, 2026

Copy link
Copy Markdown

✅ No security or compliance issues detected. Reviewed everything up to 4248ef2.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► .github/workflows/bench.yml
    Deleted (bench workflow removed)
Enhancement ► .github/workflows/candid-ui-release.yml
    Rename and update to Release workflow for candid_ui
► .github/workflows/coq.yml
    Rename job from build to coq
► .github/workflows/crates-publish.yml
    Rename and update to Crates Publish workflow
► .github/workflows/dependencies.yml
    New Dependencies workflow (cargo-deny)
Enhancement ► .github/workflows/crates-publish.yml
    Rename to Crates Publish (description updated)
Enhancement ► .github/workflows/license.yml
    Deleted License Check workflow
Enhancement ► .github/workflows/rust-bench.yml
    New Rust Bench workflow added
Enhancement ► .github/workflows/rust.yml
    Major restructuring: added path filtering changes detection, split into changes/check/fuzzing/rust, updated toolchain handling and nightly fuzzing setup, and introduced aggregate rust job
Enhancement ► .github/workflows/tools.yml
    Updated to use ubuntu-24.04, added path filters, and expanded toolchain setup
Enhancement ► README.md
    Link to candid-ui-release.yml instead of candid-ui.yml
Enhancement ► dist-workspace.toml
    Updated note to reference candid-ui-release.yml for releases
Enhancement ► deny.toml
    Updated license policy wording and allowances (candid licensing rules)
Enhancement ► rust/bench/Cargo.toml
    Workspace and linting configuration added for bench crate
Enhancement ► rust/bench/bench.rs
    Code change using repeat_n instead of repeat for deterministic bench data
Enhancement ► rust/bench/diff.py
    Added threshold handling and gated regression logging for instruction counts
Enhancement ► rust/bench/diff.py
    Updated display logic to support threshold-based regression reporting
Enhancement ► rust/bench/diff.py
    New gated reporting for regressions and final summary signaling
Enhancement ► a/.github/workflows/dependencies.yml (path referenced under new file)

@lwshang
lwshang merged commit b8c17d3 into master Jul 28, 2026
18 checks passed
@lwshang
lwshang deleted the lwshang/ci-overhaul branch July 28, 2026 12:59
lwshang added a commit that referenced this pull request Jul 28, 2026
@lwshang
Merge master into the rewrite proposal branch
47b80b7 
Picks up the CI workflow overhaul (#757). candid-ui.yml was renamed to
candid-ui-release.yml there; the reference in REWRITE.md is fixed in the
next commit.
lwshang added a commit that referenced this pull request Jul 28, 2026
@lwshang @claude
docs: follow the workflow rename from #757
2693eaa 
candid-ui.yml became candid-ui-release.yml under the naming convention
adopted in #757.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raymondk raymondk raymondk approved these changes

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

@lwshang @raymondk