Skip to content

ci: light-classify inert scripts and stop allocating macOS/Windows runners for light PRs#4025

Open
Hmbown wants to merge 1 commit into
mainfrom
ci/light-change-runner-efficiency
Open

ci: light-classify inert scripts and stop allocating macOS/Windows runners for light PRs#4025
Hmbown wants to merge 1 commit into
mainfrom
ci/light-change-runner-efficiency

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Problem

PR #4024 changed only scripts/v0867-setup-qa.sh (a headless developer QA probe wired into no CI job), yet change-detection emitted heavy=true. That forced Test (macos-latest) to run a full cargo test --workspace for 8m53s, Test (windows-latest) for 13m59s, and Mobile runtime smoke for 6m22s. A bash QA probe cannot affect any Rust build/test outcome, so all of that was wasted, and even for genuinely light PRs the macOS/Windows Test legs still allocated scarce macOS/Windows runners just to run an echo.

Fix

  1. Structural runner fix. The test and npm-wrapper-smoke jobs now use runs-on: ${{ needs.changes.outputs.heavy == 'true' && matrix.os || 'ubuntu-latest' }}. Light changes run every matrix leg on ubuntu-latest (the legs only echo their skip step); heavy changes still allocate real macOS/Windows/Linux runners exactly as before.
  2. Classification refinement. The light allowlist gains four exact paths that are provably inert or covered by always-on jobs: scripts/v0867-setup-qa.sh, scripts/release/check-versions.sh, scripts/release/check-ohos-deps.sh, scripts/check-coauthor-trailers.py. A new first case arm hard-forces heavy for the must-stay-heavy triggers (npm-wrapper-smoke.js, mobile-smoke.sh, check-provider-registry.py).
  3. Cache observability (minimal). A read-only sccache --show-stats step (continue-on-error: true) was added to each sccache-enabled job. No new cache system, no key changes.

Why safe

Context preservation. All six ruleset-required contexts are byte-identical: Lint, Test (ubuntu-latest), Test (macos-latest), Test (windows-latest), Version drift, npm wrapper smoke (ubuntu-latest). Matrix check-context names derive from job name: + matrix.os and are independent of runs-on, so redirecting a leg to ubuntu keeps its required name.

Coverage. Each newly-light path is proven covered or inert: v0867-setup-qa.sh is referenced by no Rust test, no build.rs, and no workflow (verified by grep), so no job ever runs it; check-versions.sh/check-ohos-deps.sh run in the always-on Version drift job (no needs/if); check-coauthor-trailers.py runs in the always-on non-schedule lint step. The three heavy-gated triggers are matched first and stay heavy, and the fail-safe default-heavy *) arm is retained for anything unknown. The heavy path is behaviorally unchanged.

Test plan

  • python3 -c "import yaml; yaml.safe_load(open('.github/workflows/ci.yml'))" parses cleanly and all eight jobs load.
  • Verify a PR touching only scripts/v0867-setup-qa.sh reports heavy=false, all six required contexts go green, and no macOS/Windows runner is allocated.
  • Verify a PR touching crates/** or any .rs still reports heavy=true and runs the full heavy suite on real macOS/Windows.
  • Verify a PR touching scripts/mobile-smoke.sh, scripts/release/npm-wrapper-smoke.js, or scripts/check-provider-registry.py still reports heavy=true.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PKHUCQqvi6MVgJhnQrFZ1H

Light PRs (docs / inert scripts) were allocating macOS/Windows Test legs
just to echo a skip step, and shell-script-only PRs like #4024 were
misclassified heavy and ran the full cargo-test matrix (8-14 min) for a
change that cannot affect any Rust build/test outcome.

- test + npm-wrapper-smoke: runs-on now collapses every matrix leg onto
  ubuntu-latest for light changes and uses the real matrix.os only for
  heavy changes. The six ruleset-required contexts (Lint, Test
  (ubuntu/macos/windows-latest), Version drift, npm wrapper smoke
  (ubuntu-latest)) derive from job name + matrix.os and are preserved
  because they are independent of runs-on.
- change detection: light-classify four provably-inert or always-covered
  paths (scripts/v0867-setup-qa.sh, scripts/release/check-versions.sh,
  scripts/release/check-ohos-deps.sh, scripts/check-coauthor-trailers.py);
  an ordered first case arm hard-forces heavy for the must-stay-heavy
  triggers (scripts/mobile-smoke.sh, scripts/release/npm-wrapper-smoke.js,
  scripts/check-provider-registry.py). The fail-safe default-heavy arm is
  retained for anything unknown.
- add a read-only `sccache --show-stats` observability step
  (continue-on-error) to each sccache-enabled job; no new cache system and
  no cache-key changes.

Signed-off-by: Hunter B <hmbown@gmail.com>
Copilot AI review requested due to automatic review settings July 3, 2026 23:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @Hmbown's task in 2m 36s —— View job


Reviewing PR #4025

  • Gather context (PR body, diff, custom instructions)
  • Read .github/workflows/ci.yml diff in detail
  • Check correctness of the runs-on conditional and YAML validity
  • Check classification logic (allowlist/heavy-forcing case arms)
  • Check provider/model safety implications (N/A — CI-only change, no Rust/provider code touched)
  • Post review findings

Summary

This is a well-reasoned, self-consistent CI-only change. I independently verified the key factual claims in the PR body rather than taking them on faith:

  • grep -rln v0867-setup-qa . confirms the script is referenced only by docs/evidence/*.md and the ci.yml case arm itself — no workflow, test, or build.rs runs it. Classifying it light drops zero coverage.
  • scripts/release/check-versions.sh and check-ohos-deps.sh are run by the versions job (Version drift, ci.yml:124-136), which has no needs/if gate — it always runs regardless of the heavy output. Coverage is preserved.
  • scripts/check-coauthor-trailers.py runs in the lint job's "Check harvested contributor credit" step (ci.yml:193-208), gated only on github.event_name != 'schedule', not on heavy. Coverage is preserved.
  • The three "must stay heavy" scripts (npm-wrapper-smoke.js, mobile-smoke.sh, check-provider-registry.py) are each the sole trigger of a heavy == 'true'-gated step/job (ci.yml:190-192, 341-343, 378-379), so forcing them heavy first is correct, and there's no overlap with the light list.
  • The runs-on: ${{ needs.changes.outputs.heavy == 'true' && matrix.os || 'ubuntu-latest' }} ternary is sound: GitHub Actions &&/|| return operands (JS-like), matrix.os is always one of three non-empty strings, so the expression can never resolve to empty. Required check-run names for matrix jobs are derived from job.name + the matrix key value, independent of runs-on, so this doesn't silently break required status contexts (Test (macos-latest), Test (windows-latest), etc.) — confirmed by reading how the strategy.matrix.os list itself is untouched.
  • Note: for npm-wrapper-smoke, the runs-on ternary only changes behavior on push events — on pull_request the matrix is already ubuntu-only (ci.yml:313), so the savings for that job apply to push/main builds, not PRs. This matches what the PR body actually says, just flagging it so it's not mistaken for a PR-time savings on that job.
  • sccache --show-stats steps are consistently gated with the same if: conditions as the sccache setup steps they follow (continue-on-error: true, read-only), so they can't introduce new failures or run without sccache having been enabled.
  • YAML structure/indentation is well-formed on inspection (consistent with the rest of the file's list/mapping style).

No correctness bugs, and no provider/model-prefix inference concerns apply here (this diff touches only .github/workflows/ci.yml).

Minor, non-blocking nit: the runs-on: ${{ needs.changes.outputs.heavy == 'true' && matrix.os || 'ubuntu-latest' }} ternary is duplicated verbatim in both the test and npm-wrapper-smoke jobs. A YAML anchor (&heavy-runs-on / *heavy-runs-on) could dedupe it, but duplication here is arguably more readable/greppable in a CI file, so I wouldn't block on this.

Nice attention to detail on the fail-safe ordering (must-stay-heavy matched before the light allowlist) and on proving each newly-light path is either provably inert or covered by an always-on job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants