Skip to content

ci: remove redundant CI caller rollups - #13578

Closed
teamleaderleo wants to merge 3 commits into
mainfrom
ci/remove-redundant-caller-rollups-current
Closed

teamleaderleo wants to merge 3 commits into
mainfrom
ci/remove-redundant-caller-rollups-current

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Remove the two caller-level Linux rollups that only re-check results already owned elsewhere:

  • delete linux-preflight;
  • delete the standalone tests aggregate;
  • keep macOS admission on the current changes + static-preflight gate so macOS work continues overlapping routed Linux work;
  • make required ci-status the single final owner of caller route/result consistency.

Why

Current CI already starts macOS after the cheap static gate, independently of the longer guard/web lanes. The remaining caller flow still allocates two extra Linux runners solely to reinterpret results.

Reusable workflows already publish authoritative aggregate checks for guard, web, and macOS work. The final required ci-status can enforce the caller-level route/result contract directly without two scheduling hops.

Result semantics

ci-status now requires:

  • changes and static-preflight to succeed;
  • any routed guard aggregate to succeed;
  • routed GhosttyKit verification to succeed;
  • routed web aggregate work to succeed;
  • routed macOS work to succeed;
  • malformed route outputs to fail;
  • failures/cancellations in dependencies to fail;
  • legitimately unrouted jobs to remain skippable.

macOS admission itself remains on the current early static gate, preserving overlap with routed Linux checks.

Effect

Two Linux runner allocations and two caller scheduling hops disappear from every relevant CI run while the required check stays ci-status.

Testing

Regression coverage pins:

  • absence of both old caller rollup jobs;
  • unchanged early macOS admission;
  • required router/static-preflight success;
  • routed macOS/web/guard/GhosttyKit skipped-result rejection;
  • malformed route handling;
  • legitimate unrouted skips;
  • final status ownership in ci-status.

Demo Video

N/A — CI control-plane cleanup only.

Review Trigger

@codex review
@coderabbitai review
@greptileai review
@cubic-dev-ai review

Checklist

  • Required check name remains ci-status
  • macOS keeps overlapping routed Linux work
  • Platform command bodies and test selection stay unchanged
  • Routed work must finish successfully
  • Unrouted work may skip
  • Router/static-preflight cannot silently skip
  • Both redundant Linux caller rollups are removed

Summary by cubic

Removes the redundant linux-preflight and standalone tests caller rollups, so the required ci-status job now owns all caller-level route and result verification without two extra Linux runner allocations.

  • Deletes both caller rollup jobs and moves their guard, web, GhosttyKit, and macOS route/result checks into ci-status.
  • macOS admission still happens after the cheap static gate, so macOS work keeps overlapping routed Linux work.
  • Updates the CI regression tests to pin the rollup-free behavior and the new ci-status semantics.

Written for commit 1bce00a. Summary will update on new commits.

Review in cubic

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 532d40b3-f5fd-48ce-a851-fd403a1c10ed

📥 Commits

Reviewing files that changed from the base of the PR and between 0af2e7f and 1bce00a.

📒 Files selected for processing (3)
  • .github/workflows/ci.yml
  • tests/test_ci_change_areas.py
  • tests/test_ci_linux_guard_routing.py

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.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Triage note — conflict with #13410, no duplicate.

Both PRs make large, overlapping edits to the same three files:

.github/workflows/ci.yml tests/test_ci_change_areas.py tests/test_ci_linux_guard_routing.py
#13578 (this) +65 / −171 +79 / −160 +20 / −37
#13410 (draft) +13 / −58 +96 / −182 +6 / −2

They do different things — you delete the linux-preflight and tests caller rollups and move the route/result contract into ci-status; #13410 replaces inferred change-area routing with a declarative .github/ci-areas.yml table. Both are legitimate and both should survive. But they rewrite the same regression suite from opposite ends, so whichever lands second is a real merge, not a rebase.

#13410 is a draft and is based on ci/rfc-13095-extract-macos, which has no open PR — so it is not close to landing and this PR should go first. Worth a word with its author so the area-table work is written against the post-rollup-removal shape of ci.yml rather than the current one.

One dependency: #13727 reports that macos / macOS status currently fails with invalid route compile_admitted='' and lists this PR among the runs it was observed on. Since this PR makes ci-status the single owner of caller route/result consistency, that unset-output case is now yours to get right — #13727 fixes it in ci-macos.yml specifically because ci.yml's changes outputs are under this rewrite. Coordinate so the handling does not end up in both places or neither.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Parked, not abandoned — Thornquay 💠 (triage pass, 2026-09-23)

Still wanted: both targets are still on main at a9b0329691. linux-preflight is defined at .github/workflows/ci.yml:831 and consumed at :769, :795, :826 and :948, and the standalone tests aggregate is still at :765. Nothing merged since this PR opened has removed either.

Blocked on a rebase this PR should not pay yet. It is 381 commits behind and conflicts in all three files — .github/workflows/ci.yml, tests/test_ci_change_areas.py, tests/test_ci_linux_guard_routing.py. #13804 (bound the required aggregate check, group the ungrouped PR workflows) and #13811 (follow required checks into reusable workflows) both landed in the same region of ci.yml, so the "make required ci-status the single final owner" design needs re-reading against what those two already changed before the conflict is resolved — the resolution is a design decision, not a textual merge.

Not rebasing it now: every rebase costs a full Mac CI run and the macOS pool is saturated.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Two things from an adjacent angle, both meant to save you work rather than add any.

Heads-up on a file overlap. #13924 (open, mine) adds a test class to tests/test_ci_linux_guard_routing.py — appended at the end, plus import re near the top. You edit the from test_ci_change_areas import (...) block and remove tests mid-file. The hunks look non-overlapping and my class uses none of the imports you drop (workflow_job_block, linux_preflight_needs, run_tests_gate), so this should merge either order. Flagging it so neither of us is surprised; happy to rebase mine behind yours since you are restructuring and I am appending.

While the job graph is open, ci-status is hand-listed and nothing checks it. ci.yml's ci-status names 14 jobs in needs:, and the workflow has 15 top-level jobs, so it is complete today — by coincidence rather than by construction. Add a job and forget the needs: entry and the required gate reports success without waiting on it. I looked for a test and there is not one: the ci-status assertions in tests/test_ci_change_areas.py check specific entries (macos-debounce, the suite-coverage gate, claude-wrapper) rather than completeness.

The parser to derive it already exists and is unused by any test — split_workflow_jobs() at scripts/ci/detect_ci_change_areas.py:161, in the module that test file already imports. So the guard is roughly: parse the job names, subtract ci-status itself, assert equality with its needs:, and keep an explicit allowlist for any job that deliberately should not gate. That last part matters more after this PR than before — if removing the rollups means some job legitimately should not be in needs:, an allowlist makes that a conscious line in a file rather than an omission nobody can distinguish from a mistake.

I did not build it, because you are actively changing which jobs exist and a completeness assertion written against the current set would just fight your diff. It is yours to fold in if you want it, or I can send it once this lands — say which.

Context for why I care: this is the same shape as #13924, where ci.yml retypes the guard-group list that detect_linux_guard_changes.py derives from GROUPS, and the existing routing tests appear to cover it but cannot, because they assert the script agrees with the tuple it already imports. A green lane that never ran is the failure mode #13807 and #13808 describe.

— Rockall 🪙

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

This is now conflicting (413 commits behind), and the drift did more than create textual conflicts — it invalidated the premise for one of the two deletions.

linux-preflight picked up a second consumer in #13629, which added release-admission to ci-macos.yml:

- name: Wait for routed Linux preflight
  env:
    TARGET_JOB: "linux-preflight"

That step polls the run's jobs API for a job named literally linux-preflight, 210 attempts at 5s. If the job never appears it falls through to:

print(f"Timed out waiting for {target_job}: {last_state}", file=sys.stderr)
sys.exit(1)

So deleting the job doesn't make that step skip — it makes every release-routed run spend 17.5 minutes polling for something that can't appear, then fail closed. release-build gates on needs.release-admission.result == 'success', so the universal Release compile stops running entirely.

The replacement can't be ci-status. ci-status needs macos, and release-admission lives inside the macos workflow, so pointing the poll at ci-status deadlocks.

linux-preflight is also the only job that aggregates guards + ghosttykit-release-check + web into one named verdict that completes independently of the macOS workflow — which is exactly why a reusable workflow has to reach it by name instead of by needs. So it isn't purely a caller-level rollup anymore; it's a cross-workflow synchronization point.

The tests deletion looks unaffected — main's tests job is still only route/result consistency, and ci-status can absorb it. Worth noting its logic grew a swift_packages branch since this PR's base, so whatever moves into ci-status needs that too.

Options as I see them: drop linux-preflight from the PR and land the tests deletion alone; or keep both and give release-admission a dedicated named gate to poll, which trades one Linux runner for the other rather than saving it.

Happy to do the rebase either way — say which shape you want.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Closing: as noted in this PR's own thread, linux-preflight is now a synchronization point that other workflows (release-admission) wait on by name, so the rollups can't be deleted as proposed. The tests aggregate removal can be re-cut on its own if it's still wanted.

@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 24, 2026
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