Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 83 additions & 13 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,12 @@ on:
# branch, not main — so `branches: [main]` skipped every job below for exactly
# the PRs the workflow tells people to open, while still rendering a check set
# with no failures. See tests/test_ci_workflow_coverage.rs.
# No `paths-ignore:` either. A PR whose every changed file matched that list
# produced NO check runs at all — not even a skipped Summary — and GitHub
# reported it CLEAN (PR #785 was mechanically mergeable that way). The
# expensive matrix is skipped per-job via the `changes` job below instead, so
# every PR still gets exactly one `Summary` check and it can be required.
pull_request:
paths-ignore:
- 'scripts/claude-assistant/**'
- '.github/workflows/claude*.yml'
- '.claude/**'
- '*.md'
- 'docs/**'
push:
branches: [main]
paths-ignore:
Expand Down Expand Up @@ -54,6 +53,71 @@ jobs:
# Skip expensive CI jobs on main pushes when the tree SHA already passed on a PR.
# When a PR merges via fast-forward or rebase, the merge commit has the same tree
# SHA as the PR tip. Re-running all tests on self-hosted runners is wasteful.
# Which paths make the expensive matrix meaningful. A PR touching only these
# (docs, the assistant tooling, the claude workflows) still runs `actionlint`
# and still publishes `Summary`; it just skips the VM matrix, which cannot say
# anything about those files. Gating per-job rather than per-workflow is what
# keeps `Summary` present on every PR.
changes:
name: Changes
runs-on: ubuntu-latest
outputs:
code: ${{ steps.detect.outputs.code }}
steps:
- name: Detect whether any non-doc path changed
id: detect
env:
GH_TOKEN: ${{ github.token }}
EVENT: ${{ github.event_name }}
PR: ${{ github.event.number }}
REPO: ${{ github.repository }}
run: |
set -euo pipefail
# Only a pull_request has a well-defined changed-file set here; every
# other trigger runs the full matrix rather than guess.
if [ "$EVENT" != "pull_request" ]; then
echo "code=true" >> "$GITHUB_OUTPUT"
echo "event $EVENT: running the full matrix"
exit 0
fi
files=$(gh api "repos/$REPO/pulls/$PR/files" --paginate --jq '.[].filename')
if [ -z "$files" ]; then
# An empty file list is unexpected, so fail toward running the matrix.
echo "code=true" >> "$GITHUB_OUTPUT"
echo "no changed files reported: running the full matrix"
exit 0
fi
code=false
while IFS= read -r f; do
case "$f" in
scripts/claude-assistant/*|.github/workflows/claude*.yml|.claude/*|docs/*|*.md) ;;
*) code=true; echo "code path: $f" ;;
esac
done <<< "$files"
echo "code=$code" >> "$GITHUB_OUTPUT"
echo "code_changed=$code"

# Lints every workflow file. Lives here, not in claude-lint.yml, so `Summary`
# gates it: a PR that changes only `.github/workflows/claude*.yml` skips the
# matrix, and this is then the check that can actually fail for it.
actionlint:
name: actionlint
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
- name: Download actionlint (checksum-pinned)
run: |
set -euo pipefail
curl -sSLo /tmp/actionlint.tgz \
https://github.com/rhysd/actionlint/releases/download/v1.7.10/actionlint_1.7.10_linux_amd64.tar.gz
echo "f4c76b71db5755a713e6055cbb0857ed07e103e028bda117817660ebadb4386f /tmp/actionlint.tgz" | sha256sum -c -
tar -C /tmp -xzf /tmp/actionlint.tgz actionlint
- name: Lint workflows
# shellcheck stays off until the pre-existing SC2086/SC2129 findings in
# claude.yml's run blocks are fixed; actionlint's own checks
# (expressions, action inputs, needs references, runner labels) apply.
run: /tmp/actionlint -shellcheck=

skip-check:
name: Skip Check
runs-on: ubuntu-latest
Expand Down Expand Up @@ -102,6 +166,8 @@ jobs:
# Fast checks: fmt, clippy, audit, deny
lint:
name: Lint
needs: [changes]
if: ${{ needs.changes.outputs.code == 'true' }}
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
Expand Down Expand Up @@ -150,6 +216,8 @@ jobs:
# Verifies cargo install works without source tree access
packaging:
name: Packaging
needs: [changes]
if: ${{ needs.changes.outputs.code == 'true' }}
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
Expand Down Expand Up @@ -181,6 +249,8 @@ jobs:
# instead of real Firecracker VMs. No KVM or btrfs required.
fc-mock:
name: fc-mock
needs: [changes]
if: ${{ needs.changes.outputs.code == 'true' }}
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
Expand Down Expand Up @@ -232,8 +302,8 @@ jobs:
# Self-hosted runners with nested virtualization (ARM64: c7g.metal, x86: c5.metal)
host:
name: Host-${{ matrix.arch }}
needs: [skip-check]
if: ${{ github.event.inputs.job != 'container' && needs.skip-check.outputs.skip != 'true' }}
needs: [changes, skip-check]
if: ${{ github.event.inputs.job != 'container' && needs.skip-check.outputs.skip != 'true' && needs.changes.outputs.code == 'true' }}
runs-on: [self-hosted, Linux, '${{ matrix.arch }}']
# Longest of the last 6 successful runs: 12 min. GitHub's default is SIX HOURS, which is
# how a wedged runner held a job all day on 2026-08-06 (one job stuck >5h) while the
Expand Down Expand Up @@ -471,8 +541,8 @@ jobs:
# This validates the full snapshot lifecycle: create -> persist -> restore
host-root:
name: Host-Root-${{ matrix.arch }}-${{ matrix.mode }}
needs: [skip-check]
if: ${{ github.event.inputs.job != 'container' && needs.skip-check.outputs.skip != 'true' }}
needs: [changes, skip-check]
if: ${{ github.event.inputs.job != 'container' && needs.skip-check.outputs.skip != 'true' && needs.changes.outputs.code == 'true' }}
runs-on: [self-hosted, Linux, '${{ matrix.arch }}']
# Longest of the last 6 successful runs: 43 min (arm64 SnapshotEnabled, which runs the
# whole suite twice, plus the nested + btrfs kernel builds). ~3x headroom.
Expand Down Expand Up @@ -695,8 +765,8 @@ jobs:
# Needs KVM for VM tests (container mounts /dev/kvm)
container:
name: Container-${{ matrix.arch }}
needs: [skip-check]
if: ${{ github.event.inputs.job != 'host' && needs.skip-check.outputs.skip != 'true' }}
needs: [changes, skip-check]
if: ${{ github.event.inputs.job != 'host' && needs.skip-check.outputs.skip != 'true' && needs.changes.outputs.code == 'true' }}
runs-on: [self-hosted, Linux, '${{ matrix.arch }}']
# Longest of the last 6 successful runs: 21 min. ~3x headroom.
# See the Host job comment for why the 6h default is not acceptable here.
Expand Down Expand Up @@ -823,7 +893,7 @@ jobs:
# Final summary job - runs after all test jobs
summary:
name: Summary
needs: [skip-check, lint, packaging, fc-mock, host, host-root, container]
needs: [changes, actionlint, skip-check, lint, packaging, fc-mock, host, host-root, container]
if: always()
runs-on: ubuntu-latest
steps:
Expand Down
158 changes: 158 additions & 0 deletions tests/test_ci_workflow_coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,3 +588,161 @@ fn claude_infrastructure_retry_is_trusted_bounded_and_gates_ci_fix() {
);
}
}

/// Every PR must produce exactly one `Summary` check run.
///
/// ci.yml used to carry a `paths-ignore` on its `pull_request` trigger, so a PR
/// whose every changed file matched that list produced **no check runs at all**
/// — not even a skipped Summary — and GitHub reported it CLEAN. PR #785 (a
/// two-file actions/setup-node bump touching only `.github/workflows/claude*.yml`)
/// was mechanically mergeable that way. Same claim-confusion as the base-branch
/// filter above: "no failing checks" and "the checks ran" are different claims.
///
/// The fix is per-JOB skipping, not per-workflow: the trigger fires for every
/// PR, a `changes` job decides whether the VM matrix is meaningful, and Summary
/// is always present so it can be a required check. A second workflow that also
/// emits `Summary` is NOT an acceptable substitute: `paths` fires when ANY file
/// matches while `paths-ignore` skips only when EVERY file matches, so a PR
/// touching both `src/` and a doc would emit two `Summary` check runs and make
/// the required-check result ambiguous.
#[test]
fn every_pull_request_produces_exactly_one_summary() {
let ci = parse_workflow("ci.yml");
let pull_request = triggers(&ci)
.get("pull_request")
.expect("ci.yml has no `pull_request:` trigger — PRs would get no CI at all");

if let Some(mapping) = pull_request.as_mapping() {
assert!(
!mapping.contains_key(Value::from("paths-ignore")),
"ci.yml restricts `pull_request` with `paths-ignore:`. A PR whose every changed \
file matches it then produces zero check runs and reads as CLEAN (PR #785). Skip \
the expensive jobs with the `changes` job instead, so Summary is always present."
);
assert!(
!mapping.contains_key(Value::from("paths")),
"ci.yml restricts `pull_request` with `paths:`, so PRs outside that list get no \
Summary at all"
);
}

// No other workflow may render a competing check of the same name.
let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(".github/workflows");
for entry in std::fs::read_dir(&dir).expect("read workflows dir") {
let path = entry.expect("dir entry").path();
if path.extension().and_then(|e| e.to_str()) != Some("yml") {
continue;
}
let name = path.file_name().unwrap().to_string_lossy().to_string();
if name == "ci.yml" {
continue;
}
let wf: Value = serde_norway::from_str(&std::fs::read_to_string(&path).unwrap())
.unwrap_or_else(|e| panic!("{name} is not valid YAML: {e}"));
let Some(jobs) = wf.get("jobs").and_then(Value::as_mapping) else {
continue;
};
for (_, job) in jobs {
let renders_summary = job.get("name").and_then(Value::as_str) == Some("Summary");
assert!(
!renders_summary,
"{name} also renders a check named `Summary`. Two workflows emitting the same \
check name make a required-check result ambiguous, and their path filters \
cannot be kept complementary (`paths` fires on ANY match, `paths-ignore` \
skips only on EVERY match), so a mixed PR emits both."
);
}
}
}

/// The VM matrix is skipped per-job, and only for paths it cannot speak to.
///
/// This is the other half of the fix above: dropping `paths-ignore` must not
/// mean running hours of self-hosted VM tests to validate a doc typo. Each
/// expensive job is gated on `changes.outputs.code`, and Summary keeps gating
/// them — a skipped `needs` is already treated as non-failure by the Summary
/// step, while a failed one still fails it.
#[test]
fn expensive_jobs_are_gated_on_the_changes_job() {
let ci = parse_workflow("ci.yml");
let jobs = ci
.get("jobs")
.and_then(Value::as_mapping)
.expect("ci.yml has no `jobs:` mapping");

let changes = jobs
.get(Value::from("changes"))
.expect("ci.yml has no `changes` job to decide whether the matrix is meaningful");
let detect = format!("{changes:?}");
for ignored in [
"scripts/claude-assistant/",
".github/workflows/claude",
".claude/",
"docs/",
"*.md",
] {
assert!(
detect.contains(ignored),
"the `changes` job no longer recognises `{ignored}` as a path the VM matrix cannot \
speak to, so PRs touching only it will run the full matrix"
);
}

for job in [
"lint",
"packaging",
"fc-mock",
"host",
"host-root",
"container",
] {
let cond = jobs
.get(Value::from(job))
.and_then(|j| j.get("if"))
.and_then(Value::as_str)
.unwrap_or_else(|| panic!("ci.yml job `{job}` has no `if:` gate"));
assert!(
cond.contains("needs.changes.outputs.code == 'true'"),
"ci.yml job `{job}` is not gated on the `changes` job, so a docs-only PR runs it"
);
}
}

/// `.github/workflows/claude*.yml` changes must get a check that can fail.
///
/// Those files are exactly the ones the VM matrix says nothing about, so they
/// skip it — which must not decay into "nothing checks them". `actionlint`
/// lives in ci.yml (not in claude-lint.yml) precisely so `Summary` gates it:
/// for a claude-workflow-only PR it is the one gating job that still runs.
#[test]
fn workflow_changes_get_a_relevant_gated_check() {
let ci = parse_workflow("ci.yml");
let jobs = ci
.get("jobs")
.and_then(Value::as_mapping)
.expect("ci.yml has no `jobs:` mapping");

let actionlint = jobs.get(Value::from("actionlint")).expect(
"ci.yml no longer defines an `actionlint` job, so a workflow-only PR skips the \
matrix and nothing else can fail for it",
);
assert!(
actionlint.get("if").is_none(),
"the `actionlint` job must not be gated on `changes`: workflow-only PRs are exactly \
the case it exists to cover"
);

let needs: Vec<String> = jobs
.get(Value::from("summary"))
.and_then(|s| s.get("needs"))
.and_then(Value::as_sequence)
.expect("ci.yml `summary` job has no `needs:` list")
.iter()
.map(|v| v.as_str().expect("needs entry").to_string())
.collect();
assert!(
needs.iter().any(|n| n == "actionlint"),
"`actionlint` is defined but is not in `summary.needs`, so Summary can go green while \
it fails"
);
}
Loading