Repository navigation
ci(OMN-14127): run required-context CI aggregators (CI Summary / Security Gate) on GitHub-hosted runners so they never wedge under self-hosted saturation - #1870
Conversation
…s on GitHub-hosted runners The required branch-protection contexts "CI Summary" and "Security Gate" are emitted by needs-gated jobs in ci.yml. A needs-gated job gets no GitHub check-run until its needs reach a terminal state. Under self-hosted (omnibase-ci) fleet saturation the aggregators either (a) sat queued on the fleet until the whole run was cancelled — posting no terminal status — or (b) were never instantiated because an upstream never terminalized. Either way the required context is ABSENT and the PR wedges BLOCKED forever with 0 failing / 0 pending checks and no auto-recovery (verified: PR #1864, run 28839961159; recurrence on #1867/#1863/#1868). Move the result-reading aggregation layer off the self-hosted fleet onto GitHub-hosted ubuntu-latest so it never queues behind saturation: - ci-summary (required "CI Summary") - security-gate (required "Security Gate") - quality-gate, tests-gate (feed CI Summary) - security-python, detect-secrets (Security Gate leaves; pure static scans (bandit / detect-secrets) with no self-hosted/LAN dependency — makes the whole Security Gate subtree hosted so that required context is starvation-immune) These jobs do only static/result-reading work (echo + needs.*.result), so hosting them carries no LAN/service dependency. `if: always()` and every job name are unchanged byte-for-byte (a rename would itself name-mismatch branch protection and wedge every PR). Live required contexts on dev branch protection are "CI Summary" and "Security Gate" (verified via the protection API) — the ticket title's "Tests Gate" is not a required context, but it is moved too since it feeds CI Summary. This is the workflow-structure half of the fix; the runner-fleet capacity half is tracked under OMN-13932 / OMN-14080.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughCI jobs now run on fixed ChangesCI orchestration updates
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
2853-2903: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ci-summarycan still wedge behind self-hosted jobs
complianceandcontract-compliancestill default to["self-hosted","omnibase-ci"]on trusted CI, soci-summarycan still fail to reach a terminal state through itsneedschain. If this check is required for branch protection, the same saturation wedge remains unless those jobs move off the self-hosted fleet or are removed from the blocking dependency set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 2853 - 2903, The ci-summary aggregator can still be blocked by self-hosted jobs because it waits on compliance and contract-compliance, which default to self-hosted runners on trusted CI. Update the ci-summary job in the workflow so it no longer depends on blocking self-hosted jobs, either by moving compliance and contract-compliance onto a GitHub-hosted runner or by removing them from ci-summary’s needs list if they are not required for the branch-protection context. Keep the required terminal-status behavior aligned with the other gate jobs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 2853-2903: The ci-summary aggregator can still be blocked by
self-hosted jobs because it waits on compliance and contract-compliance, which
default to self-hosted runners on trusted CI. Update the ci-summary job in the
workflow so it no longer depends on blocking self-hosted jobs, either by moving
compliance and contract-compliance onto a GitHub-hosted runner or by removing
them from ci-summary’s needs list if they are not required for the
branch-protection context. Keep the required terminal-status behavior aligned
with the other gate jobs.
…starvation-immune)
Operator-approved follow-up to the hosted-aggregator change. Converts the
`ci-summary` job from a `needs`-gated aggregator into a NO-`needs`, GitHub-hosted,
fail-closed poller so the required `CI Summary` context is immune to self-hosted
fleet saturation.
Why: a `needs`-gated job gets no GitHub check-run until its `needs` reach a
terminal state. Under saturation the gate jobs never terminalized, so the old
needs-based `CI Summary` was ABSENT forever (PR wedged BLOCKED, no auto-recovery).
With NO `needs` the poller's check-run instantiates immediately — the required
context can never be absent — and it always posts a terminal success/failure
within a bounded deadline.
Design (fail-closed, default-deny):
- `ci-summary`: runs-on ubuntu-latest, `if: always()`, NO `needs`, permissions
actions:read + contents:read, timeout 100m. Polls
repos/.../runs/<id>/jobs?filter=all every 45s up to a 90m deadline (under the
observed ~167m run-cancel window) and calls scripts/ci/ci_summary_gate.py.
- scripts/ci/ci_summary_gate.py: pure-stdlib verdict. SUCCESS only when (a) the
default-deny sweep finds no present+completed non-allowlisted job with a
conclusion outside {success,skipped}, AND (b) every aggregate gate
(Quality/Tests/Security Gate + Contract Compliance[ Check]) is present+
completed+good. Gating on the aggregate gates (themselves if:always fail-closed
aggregators over their leaves) is what prevents a FALSE GREEN before late-created
jobs (detect-changes → test → *-gate) are even instantiated. Missing/pending
gate → PENDING → poll again; deadline → FAILURE (fail-closed). Latest-attempt
dedup handles `gh run rerun --failed`. Exit 0/1/2 = success/fail/pending.
- tests/ci/test_ci_summary_gate.py: 19 unit tests pinning the fail-closed verdict
(gate failure/cancel/absent, leaf-failure-before-gates, allowlist ignored,
self excluded, partial-rerun dedup, empty-run-not-vacuous-green, CLI exit codes).
The name `CI Summary` is unchanged byte-for-byte (it IS the required context).
No branch-protection change. Pure hosted API polling — zero self-hosted/LAN dep.
…2) does not kill the loop
GitHub runs the step as `bash -e {0}`; the gate script's exit 2 (PENDING) was
terminating the step on the first poll instead of continuing to poll. `set +e`
lets the explicit case-statement handle 0/1/2. (Poller already proven to
instantiate immediately on ubuntu-latest and post a terminal status.)
Update — fail-closed
|
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical/error findings | No |
blocked |
CRITICAL or ERROR findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)
The prior run_id (28899626848) is permanently un-greenable: after the
self-hosted fleet cancelled attempt 1, the no-needs CI Summary poller
(scripts/ci/ci_summary_gate.py over runs/{id}/jobs?filter=all) keeps
seeing prior-attempt cancelled jobs that have not yet re-instantiated in
the new attempt and fails closed at ~15-21s on every rerun. A fresh
run_id (run_attempt=1) has no stale cancelled jobs to poison the
default-deny sweep. No source change.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 2901-2907: The job-fetch retry path in the CI workflow skips the
internal deadline check because it `continue`s immediately after a failed `gh
api`/`jq` fetch. Update the retry loop around the `jobs.json` fetch so the
90-minute deadline is evaluated even when the fetch fails, either by checking
the deadline before each retry `continue` or by routing the failure branch
through the same deadline/timeout handling used later in the loop. Keep the
behavior tied to the existing `attempt`, `POLL_INTERVAL_SECONDS`, and
`jobs.json` fetch logic.
In `@scripts/ci/ci_summary_gate.py`:
- Around line 124-157: The default-deny sweep in evaluate() is losing failures
because dedup_latest() collapses same-name rows for the same run_attempt before
the sweep runs. Update the logic so current-attempt duplicate job rows are
preserved for the sweep, or perform the failure scan on the raw jobs list while
still using dedup_latest() only where a collapsed view is actually needed.
Reference dedup_latest(), evaluate(), and sweep_failures when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1d673d8-33a8-4d45-9650-acc33a50fede
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/no-faked-boundary.ymlscripts/ci/ci_summary_gate.pytests/ci/test_ci_summary_gate.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/no-faked-boundary.yml
Summary
Fixes the permanent-wedge class in
OMN-14127: the required branch-protection contextsCI SummaryandSecurity Gatenever post any check-run under self-hosted fleet saturation, leaving PRsBLOCKEDforever with 0 failing / 0 pending checks and no auto-recovery.Closes OMN-14127 (workflow-structure half; runner-fleet capacity half is OMN-13932 / OMN-14080).
Verified root cause (evidence, not assumption)
A
needs-gated job gets no GitHub check-run at all until itsneedsreach a terminal state. Confirmed live:gh pr checkson fix(OMN-14086): diff-scope aislop-sweep (full-tree→changed-files, fail-closed) #1867/ci(OMN-14080): remove merge_group triggers from 24 non-required workflows (Lever 1 fan-out) #1863/fix(OMN-13940): unquarantine executable merge sweep cron #1861)CI Summary/Security Gate/Tests Gate/Quality Gateproduce zero rows — neverpending, neverskipped. Standalone single-job contexts (contract-validation,deploy-gate) showpendingbecause they instantiate immediately (noneeds).28839961159): theif: always()gates (quality-gate/security-gate) became eligible when their upstreams cancelled, then sat queued on the self-hosted fleet until the whole run was cancelled ~2h47m later — posting no terminal status; andci-summary/tests-gatewere never instantiated becausequality-gatenever terminalized before run-cancellation.mergeStateStatus: BLOCKEDindefinitely with no timeout/retry.Note: the ticket title says
CI Summary/Tests Gate, but the live required contexts ondevbranch protection areCI SummaryandSecurity Gate(Tests Gateis not required). Verified viagh api repos/OmniNode-ai/omniclaude/branches/dev/protection/required_status_checks.Fix — relocate the result-reading aggregation layer to GitHub-hosted
ubuntu-latestThese jobs do only static / result-reading work (
echo+needs.*.result) with no LAN/service dependency, so they never need a scarce self-hosted slot. Moving them off the fleet means they get a runner immediately and reach a terminal state as soon as theirneedsresolve — instead of stalling in the queue and being cancelled.name:(context)ci-summaryif: always()→ always terminal once its gateneedsresolvesecurity-gatesecurity-pythonbanditstatic scan, hosted-safe → makes the Security Gate subtree fully starvation-immunedetect-secretsdetect-secretsstatic scan, hosted-safequality-gatetests-gateif: always()preserved on all gate aggregators; verdict logic untouched.test(matrix),hooks-tests,golden-chain-live, the 35 quality leaves, etc. stay on the self-hosted fleet. Only the lightweight aggregation layer + the two static security leaves move.required_status_checkschange in this PR (operator-gated step 2 — see below).Residual (honest scope boundary)
CI Summarystillneedsthe self-hosted quality/tests leaf subtree. If those leaves sitqueuedindefinitely (never even cancel),quality-gate/tests-gate— and thereforeCI Summary— still cannot instantiate. That is the runner-fleet capacity problem (OMN-13932 / OMN-14080), not a workflow-structure problem. This PR fully removes the aggregation-layer stall (the observed failure in the ticket trace: gates sitting queued and getting cancelled) and makesSecurity Gatefully starvation-immune (its entire subtree is now hosted). FullCI Summarystarvation-immunity would require either moving its whole leaf subtree hosted or converting it to a no-needspoller — flagged as a possible fast-follow, not done here.Local verification
python3 -c "import yaml; yaml.safe_load(...)"→ parses, 60 jobs.actionlint .github/workflows/ci.yml→ 5 findings, identical toorigin/devbaseline (all pre-existing shellcheck style/info in untouchedrun:blocks) — zero new findings.pre-commit run --files .github/workflows/ci.yml→ exit 0, 0 failures (incl.ban-hardcoded-runner-ip, concurrency-primitive-guard, self-gating-workflows).Evidence
Summary by CodeRabbit
Evidence-Ticket: OMN-14127
Evidence-Source: OCC#3722