diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3c8a76ca9..a93e22f70 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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: @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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. @@ -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. @@ -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: diff --git a/tests/test_ci_workflow_coverage.rs b/tests/test_ci_workflow_coverage.rs index 904b55143..8b1744697 100644 --- a/tests/test_ci_workflow_coverage.rs +++ b/tests/test_ci_workflow_coverage.rs @@ -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 = 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" + ); +}