Repository navigation
ci: aggregate required gates (stop branch-protection desync on renames/splits) - #6519
Merged
Merged
Conversation
…protection Branch protection requires status checks by literal name, but recent CI churn (unit-test sharding renamed the `tests` job to `app-host unit tests (N/4)` in #6464; jobs split across ci.yml and test-ios.yml; new suites added) kept moving those names out from under the static required-checks list — stranding old names ("expected" forever, blocking every PR) and leaving new suites ungated. Fix: gate via aggregate summary jobs that reference suites by their job KEY (immune to display-name/shard changes), one per workflow. - ci.yml `tests-required-status` (reported as `tests`, already required): now also needs `swift-package-tests` and `agent-session-web-resources`, so a failure in either blocks merge. Skipped (path-filtered) is still allowed. - test-ios.yml: new `ios-tests` aggregate over `detect-ios-changes`, `package-conventions-lint`, `mobile-core-package`, `ios-simulator`, same skip-tolerant logic. Settings follow-up (after merge): add `ios-tests` to the main ruleset's required status checks. The `tests` change needs no settings change (same name). Optional cleanup: the individual web/release contexts can stay or be folded into the aggregates later. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThree CI workflow updates refactor test job naming and extend gate aggregation logic. In ChangesCI Gate Aggregation Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 21 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (21 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
Greptile SummaryThis PR addresses a recurring class of CI desync by introducing stable aggregate gate jobs whose
Confidence Score: 5/5Safe to merge; the only post-merge action needed is manually adding All changes are CI workflow mechanics — renaming job keys, wiring aggregate gates, and updating tests to match. The Python guard logic in both aggregate jobs correctly distinguishes No files require special attention; all four changed files are CI/test infrastructure with no production code impact. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph ci.yml
changes --> app-host-unit-tests
changes --> swift-package-tests
changes --> agent-session-web-resources
app-host-unit-tests --> tests["tests (aggregate gate)"]
swift-package-tests --> tests
agent-session-web-resources --> tests
changes --> tests
tests --> ci-status
app-host-unit-tests --> ci-status
swift-package-tests --> ci-status
agent-session-web-resources --> ci-status
end
subgraph test-ios.yml
detect-ios-changes --> ios-tests["ios-tests (aggregate gate)"]
package-conventions-lint --> ios-tests
mobile-core-package --> ios-tests
ios-simulator --> ios-tests
end
tests -->|"required check: 'tests'"| BP[Branch Protection]
ios-tests -->|"required check: 'ios-tests' (add after merge)"| BP
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph ci.yml
changes --> app-host-unit-tests
changes --> swift-package-tests
changes --> agent-session-web-resources
app-host-unit-tests --> tests["tests (aggregate gate)"]
swift-package-tests --> tests
agent-session-web-resources --> tests
changes --> tests
tests --> ci-status
app-host-unit-tests --> ci-status
swift-package-tests --> ci-status
agent-session-web-resources --> ci-status
end
subgraph test-ios.yml
detect-ios-changes --> ios-tests["ios-tests (aggregate gate)"]
package-conventions-lint --> ios-tests
mobile-core-package --> ios-tests
ios-simulator --> ios-tests
end
tests -->|"required check: 'tests'"| BP[Branch Protection]
ios-tests -->|"required check: 'ios-tests' (add after merge)"| BP
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+368
to
+415
| ios-tests: | ||
| name: ios-tests | ||
| # Aggregate gate for this workflow's iOS suites. Add this check to the branch | ||
| # protection required list (it is the iOS sibling of ci.yml's "tests" gate). | ||
| # References each suite by job KEY via needs:, so renaming or sharding a job | ||
| # never desyncs the required-checks list. Add new iOS jobs to needs: here. | ||
| needs: | ||
| - detect-ios-changes | ||
| - package-conventions-lint | ||
| - mobile-core-package | ||
| - ios-simulator | ||
| if: ${{ always() }} | ||
| runs-on: ${{ vars.LINUX_RUNNER || 'warp-ubuntu-latest-x64-4x' }} | ||
| timeout-minutes: 5 | ||
| steps: | ||
| - name: Check iOS test routing | ||
| env: | ||
| IOS_NEEDS: ${{ toJSON(needs) }} | ||
| run: | | ||
| python3 - <<'PY' | ||
| import json | ||
| import os | ||
| import sys | ||
|
|
||
| needs = json.loads(os.environ["IOS_NEEDS"]) | ||
|
|
||
| # The routing job must succeed for its should_run/should_lint outputs to | ||
| # be trustworthy; the suites below run or skip based on those outputs. | ||
| if needs["detect-ios-changes"]["result"] != "success": | ||
| print(f"detect-ios-changes: {needs['detect-ios-changes']['result']}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| # A suite that opted out via the routing filter reports "skipped", which | ||
| # is fine; a suite that actually ran and failed must block the merge. | ||
| allowed = {"success", "skipped"} | ||
| bad = { | ||
| name: data["result"] | ||
| for name, data in sorted(needs.items()) | ||
| if name != "detect-ios-changes" and data["result"] not in allowed | ||
| } | ||
| if bad: | ||
| for name, result in bad.items(): | ||
| print(f"{name} did not pass: {result}", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| for name, data in sorted(needs.items()): | ||
| print(f"{name}={data['result']}") | ||
| PY |
Contributor
There was a problem hiding this comment.
ios-tests required-check will block PRs that never trigger test-ios.yml
test-ios.yml has a paths: trigger filter. For a PR that touches only web/**, a server-side Go file, or any other path outside the listed patterns, the entire workflow never runs — meaning ios-tests produces no check status at all. GitHub reports the check as "Expected" (not "skipped"), and a required check that is absent blocks the merge.
The PR description says "skip = pass here," but that only describes jobs that opt out inside a running workflow (e.g., ios-simulator skipping because should_run=false). It does not cover the case where the workflow itself never triggers. If ios-tests is added to the ruleset's required list under the default "must be present and passing" mode, every web-only, Go-only, or other non-iOS-path PR will be permanently blocked.
The safe configuration options are: (a) switch the GitHub Ruleset entry to "required if triggered" / allow-if-skipped mode, or (b) remove the paths: filter from the on: pull_request: trigger so the workflow always runs and internal job conditions handle the skip logic (the same model ci.yml uses).
Removes the confusing name/key crossover left by #6464+#6474: a job KEYED `tests` that reported as "app-host unit tests", plus a gate keyed `tests-required-status` that reported as `tests`. Now the names line up: - `app-host-unit-tests` (key) -> reports "app-host unit tests (N/4)" — the suite - `tests` (key) -> reports "tests" — the required aggregate gate No settings change: the gate still reports under the required name `tests`. The two `needs:` references to the old matrix key (the gate and ci-status) and the gate's needs["..."] lookup are updated accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The job-key rename moved the macOS app-host matrix to `app-host-unit-tests` and gave the key `tests` to the required aggregate gate. Update the guards that asserted on the old keys: - test_ci_self_hosted_guard.sh: assert the paid-macOS-runner requirement against `app-host-unit-tests` (the matrix), not `tests` (now a linux gate). - test_ci_change_areas.py: ci-status routed-jobs list and the gate-block test now reference `app-host-unit-tests` (matrix) and `tests` (gate). All workflow-guard-tests steps pass locally (self-hosted guard, change-areas, sharding validator). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Branch protection matches required status checks by literal name, and that list is hand-maintained. The recent CI burst kept changing names out from under it:
testsjob → reported asapp-host unit tests (N/4), so the requiredtestscontext went "expected" forever and blocked every PR built on top of it (until Route agent session web resource CI #6474 added atestssummary back).test-ios.yml; new suites (swift-package-tests,agent-session-web-resources) were added — none added to the required list, so they run but don't gate.This is a recurring class of breakage: every rename / shard / workflow-split silently desyncs the required-checks name list.
What
Gate via aggregate summary jobs that reference suites by their job KEY through
needs:(immune to display-name and shard changes), one per workflow:ci.yml→tests-required-status(reported astests, already required): now alsoneeds:swift-package-tests+agent-session-web-resources. A real failure in either blocks merge; a path-filtered skip is allowed.test-ios.yml→ newios-testsaggregate:needs:detect-ios-changes,package-conventions-lint,mobile-core-package,ios-simulator, same skip-tolerant logic.Both mirror the existing
ci-status/tests-required-statuspattern.Settings change required (after merge)
ios-teststo themainruleset's required status checks. Do this after this PR merges, so the job exists first.testschange needs no settings change (same check name).Policy note
ios-testsmakes a real iOS suite failure block iOS-touching PRs (iOS jobs skip on non-iOS PRs viadetect-ios-changes, and skip = pass here). If iOS sim flakiness should stay advisory, dropios-simulatorfrom theios-testsneeds:and keep onlymobile-core-package+package-conventions-lint. Your call.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Aggregate CI gates to keep branch protection stable when jobs are renamed or sharded. Expands the
testsgate, adds a newios-testsgate, and renames the unit-test matrix job key toapp-host-unit-tests; updates CI guard tests and wiring to match.Refactors
ci.yml:testsgate now needsapp-host-unit-tests,swift-package-tests, andagent-session-web-resources; updatedci-statusneeds to reference the new keys.test-ios.yml: addsios-testsgate overdetect-ios-changes,package-conventions-lint,mobile-core-package, andios-simulator.tests→app-host-unit-tests; aggregate is keyedtests. Updatedtests/test_ci_change_areas.pyandtests/test_ci_self_hosted_guard.sh.Migration
ios-teststo the main ruleset’s required checks.testsneeds no change.Written for commit 9c882ec. Summary will update on new commits.
Summary by CodeRabbit
successorskipped.success/skipped.