Skip to content

ci: finish successful aggregate checks without a runner queue - #29136

Merged
nwparker merged 3 commits into
mainfrom
nwparker/ci-aggregate-fast-path
Oct 15, 2026
Merged

nwparker merged 3 commits into
mainfrom
nwparker/ci-aggregate-fast-path

Conversation

@nwparker

@nwparker nwparker commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

When all required checks have the expected results, finish the aggregate check without waiting for another runner. A failed or unexpectedly skipped prerequisite still runs the existing checker and fails the PR.

What Changed

The existing path-detection job validates its routing values. The final job then compares every prerequisite with that routing before allocating a runner. A valid combination skips the aggregate job, which GitHub accepts for required checks. Any mismatch, missing evidence, or noncanonical routing uses the original shell and environment unchanged. The required check name, dependency list, and cancellation behavior stay the same.

The routing marker uses exact lowercase values because GitHub expression equality ignores case. Explicit domain checks also catch outputs removed after validation. The new dev-only @actions/expressions dependency tests the actual Actions expression semantics; it adds no transitive packages or production imports.

Why

The last qualified run queued the aggregate for 821 seconds, then spent 9 seconds reporting results already known by GitHub. Moving the valid-result decision into the job condition removes that allocation. The existing shell remains the conservative fallback instead of replacing the checker or weakening success-versus-skip requirements.

That 821-second observation is one run, not a promised average or a whole-pipeline benchmark. Other jobs and queue delays can still determine completion time.

Linked Issue

N/A — maintainer CI performance work.

Visual Proof

N/A — CI workflow and tests only; no product UI changes.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated

On macOS: 7 focused files / 117 cases passed, including all 8,192 valid routing combinations, individual dependency mismatches, the full preflight selector pair, cancellation, readiness reuse, and masked/malformed outputs. Actual Bash checks cover 158 routing-marker cases and 366 aggregate parity cases, batched to keep test cost low. These are not every mismatch in every mixed routing combination.

Typecheck, changed-code quality, changed-file Oxlint/Oxfmt, and independent static review passed. The existing fallback shell and environment are byte-identical to the base.

Hosted negative qualification passed: run 38723009907 deliberately skipped a selected preflight, allocated the aggregate runner, and failed with preflight: expected success, got skipped and exit code 1. The normal preflight condition is restored, the branch is based on current main, and all seven implementation files exactly match the locally tested and independently reviewed bytes. Final qualification passed at exact head 377fc9025ec4a0fa7fc414ccb6487ca8837301e3 in run 38724756703. All 46 check results completed: 36 successful and 10 legitimate skips. All 12,308 unit roots were assigned and executed exactly once; all 33 changed-root cases passed, with zero unhandled errors. Seven artifact digests, all five unit logs, detector routing and every selected prerequisite were independently verified. The aggregate completed as skipped with no steps or runner allocation. The latest-main combination, including Mobile routing, also passed 137 focused cases locally; all seven PR paths are disjoint from upstream changes. The whole combined tree has not been hosted-tested.

Official skipped-job behavior: https://docs.github.com/en/actions/how-tos/write-workflows/choose-when-workflows-run/control-jobs-with-conditions

Review

Independent review checked all 12 conditional prerequisites, exact routing validation, missing/masked outputs, cancellation, readiness reuse, and expression-versus-shell semantics. GitHub controls prerequisite result values through its documented success/failure/cancelled/skipped enum.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

No application behavior, host ownership, remote protocol, native runtime, or platform assumptions change. The existing aggregate shell continues to execute on Linux; expression tests run without Bash on Windows, while shell tests retain their existing Windows skip.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • N/A visual proof with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform and SSH/remote impact considered
  • Final exact-head hosted qualification completed

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

The workflow now validates that required routing outputs are exactly lowercase true or false. It skips verify only when routing is canonical and selected job results match their routing values; otherwise, it runs detailed validation. The preflight job is disabled. New helper code evaluates the aggregate gate, and tests cover routing and gate behavior.

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to de73c

Code PRs that select static analysis or typecheck cannot pass the required checks while preflight is disabled. Restore its condition before merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: successful aggregate checks can finish without allocating a runner.
Description check Passed The description is complete and focused. It covers the user-facing change, rationale, implementation, testing, hosted qualification, compatibility considerations, and checklist. The Linked Issue secti…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 463cdf1a-d578-4634-a603-2f7f6e70f4ca
📥 Commits

Reviewing files that changed from the base of the PR and between cdf4b38 and de73c727c09d022cdfc5432cffc213dfd1afe357.

⛔ Files ignored due to path filters: 1
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing: 6
  • .github/workflows/pr.yml
  • config/scripts/pr-aggregate-gate-context.mjs
  • config/scripts/pr-aggregate-gate.test.mjs
  • config/scripts/pr-ready-check-gate.test.mjs
  • config/scripts/pr-ready-check-reuse.test.mjs
  • package.json

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Comment thread .github/workflows/pr.yml Resolved
@nwparker
nwparker force-pushed the nwparker/ci-aggregate-fast-path branch from de73c72 to 377fc90 Compare October 15, 2026 11:04
@nwparker
nwparker merged commit 0235673 into main Oct 15, 2026
46 checks passed
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.

1 participant