Skip to content

Require hosted CI for generator PRs #4149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
justin808 merged 3 commits into from
Jun 21, 2026
Merged

Require hosted CI for generator PRs #4149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69e34e6
Require hosted CI for generator PRs
justin808 Jun 20, 2026
d7e279c
Cover hosted CI gate edge cases
justin808 Jun 21, 2026
7228de8
Update Pro async prop streaming expectation
justin808 Jun 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/read-me.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ job-level conditions:

- Ordinary PR updates run the required gate only unless `ready-for-hosted-ci`,
`force-full-hosted-ci`, or a release-target base branch allows hosted jobs.
- Generator-sensitive PRs are stricter: if change detection sets
`run_generators=true`, `ci-required / required-pr-gate` fails on ordinary PRs
until hosted CI is requested.
- `ready-for-hosted-ci` permits hosted jobs, but the change detector still
selects applicable suites.
- `force-full-hosted-ci` or `workflow_dispatch` with `force_full_hosted: true`
Expand Down
20 changes: 19 additions & 1 deletion .github/workflows/ci-required.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ on:

permissions:
contents: read
issues: read

concurrency:
group: ci-required-${{ github.event.pull_request.number || github.ref }}
Expand All @@ -34,16 +35,33 @@ jobs:
bin/ci-local \
bin/ci-rerun-failures \
bin/request-hosted-ci \
script/ci-changes-detector
script/ci-changes-detector \
script/ci-required-hosted-gate
do
bash -n "$script"
done

- name: Run CI gate tests
run: |
bash script/ci-required-hosted-gate-test.bash

- name: Select hosted CI mode
id: hosted-ci
uses: ./.github/actions/hosted-ci-selectors

- name: Run changed-files detector
id: changes
run: |
base_ref="${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.before || 'origin/main' }}"
script/ci-changes-detector "$base_ref"

- name: Enforce hosted CI for generator changes
env:
RUN_GENERATORS: ${{ steps.changes.outputs.run_generators }}
SHOULD_RUN_HOSTED_CI: ${{ steps.hosted-ci.outputs.should_run_hosted_ci }}
run: |
script/ci-required-hosted-gate

- name: Lightweight repo sanity check
run: |
test -f Gemfile
Expand Down
1 change: 1 addition & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ Agents should recommend PR labels based on change complexity and risk. The goal

- **Default: no CI-expansion label.** For docs-only changes, focused tests, small isolated fixes, and refactors with no cross-package behavior change, rely on `ci-required / required-pr-gate` plus local verification during review.
- **Use `ready-for-hosted-ci`** (or ask a maintainer to comment `+ci-run-hosted`) when the PR is ready for hosted GitHub Actions confirmation. This runs the hosted workflows for the current head SHA, but `script/ci-changes-detector` still chooses the applicable suites. Opening a draft PR or requesting code review does not by itself mean hosted CI should run.
- **Generator-sensitive PRs require hosted CI.** When `script/ci-changes-detector` sets `run_generators=true`, `ci-required / required-pr-gate` fails on ordinary pull requests until hosted CI is requested with `+ci-run-hosted`, `bin/request-hosted-ci`, or a maintainer/user-token `ready-for-hosted-ci` label. This keeps generator changes from merging after only the lightweight gate; merge queue and release-target branches already run hosted CI automatically.
- **Use `force-full-hosted-ci`** only when a maintainer intentionally wants to bypass optimized suite selection and run every hosted suite, for example while validating CI detector changes, package manager or runtime floor changes, release/build/publishing logic, broad generator output, or another cross-cutting change where path selection itself is part of the risk. Prefer `+ci-force-full`, which also applies `ready-for-hosted-ci` and dispatches the workflows for the current head SHA.
- **Use `benchmark`** (or a suite-specific `benchmark-core` / `benchmark-pro` / `benchmark-pro-node-renderer`) for performance-sensitive changes: server rendering paths, Node renderer, caching, bundle generation, asset serving/precompile behavior, concurrency/pooling, or anything expected to affect throughput, latency, memory, or bundle size. Benchmarks are opt-in on PRs: without a `benchmark*` label no suite runs, because per-PR benchmark numbers are informational only and noise-dominated on shared CI runners. They still run on push to `main` to keep the Bencher dashboard and PR-comparison baseline current, but automatic regression-issue filing is disabled by default (#4071): on shared runners those alerts were ±50-125% noise that filed false-positive issues (#4038-#4044), so the trustworthy signal now comes from the dedicated local runner (`benchmarks/run-local-benchmark.rb`, #4073). Set the repo variable `BENCHMARK_REGRESSION_ISSUES_ENABLED=true` to restore automatic filing. `ready-for-hosted-ci` and `force-full-hosted-ci` do not trigger benchmarks; use benchmark labels separately when performance evidence matters. Use `hosted-ci-no-benchmarks` only to suppress an explicit benchmark label on CI/tooling PRs that cannot move runtime performance.
- **Remove hosted readiness when no longer needed** with `+ci-stop-hosted` if the PR returns to active iteration. Use `+ci-stop-full` when only the force-full override should be removed and optimized hosted CI should remain.
Expand Down
8 changes: 8 additions & 0 deletions internal/contributor-info/ci-optimization.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,19 @@ The `required-pr-gate` job intentionally stays lightweight:
- validates workflow YAML can be loaded by Ruby
- syntax-checks CI shell helpers
- runs `script/ci-changes-detector`
- fails ordinary pull requests with generator-sensitive changes until hosted CI
is requested
- checks basic repository structure

Repository branch protection should require `ci-required / required-pr-gate`.
Do not require heavyweight hosted jobs for ordinary review pushes unless the
repository intentionally wants every PR update to allocate hosted runners.
Generator-sensitive changes are the standing exception: when
`script/ci-changes-detector` sets `run_generators=true`, the required gate
blocks ordinary pull requests until `+ci-run-hosted`, `bin/request-hosted-ci`,
or a maintainer/user-token `ready-for-hosted-ci` label requests hosted CI.
Merge queue, push-to-main, and release-target runs already satisfy hosted
eligibility.

## Hosted CI Modes

Expand Down
2 changes: 1 addition & 1 deletion packages/react-on-rails-pro-node-renderer/tests/incrementalHtmlStreaming.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ it('raises an error if a specific async prop is not sent', async () => {

request.end();
await expect(getNextChunk(request)).resolves.toContain(
'The async prop \\"researches\\" is not received. Ensure to send the async prop from ruby side',
'The async prop \\"researches\\" was not received. Make sure you send it from the Rails side',
);

await expect(getNextChunk(request)).rejects.toThrow('Stream Closed');
Expand Down
24 changes: 24 additions & 0 deletions script/ci-required-hosted-gate
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#!/usr/bin/env bash
# Fails the required PR gate when generator-sensitive changes have not requested
# hosted CI. Keep this small: ci-required must stay fast and deterministic.

set -euo pipefail

event_name="${GITHUB_EVENT_NAME:-}"
run_generators="${RUN_GENERATORS:-false}"
should_run_hosted_ci="${SHOULD_RUN_HOSTED_CI:-false}"

if [ "$event_name" = "pull_request" ] &&
[ "$run_generators" = "true" ] &&
[ "$should_run_hosted_ci" != "true" ]; then
cat >&2 <<'MSG'
Generator changes require hosted CI before merge.

This PR changes generator-sensitive paths, but hosted CI has not been requested.
Comment +ci-run-hosted on the PR, or add the ready-for-hosted-ci label with a maintainer/user token.

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Ensure +ci-run-hosted reruns the required gate

For generator PRs that use the documented +ci-run-hosted path, this instruction does not actually unblock the failed required check: the comment workflow adds ready-for-hosted-ci with GITHUB_TOKEN, and the repo docs note that such label writes do not start new pull_request workflow runs, while triggerHostedCi dispatches only the hosted workflows, not ci-required.yml. In that scenario the last ci-required / required-pr-gate run remains failed until someone pushes, manually reruns/dispatches the gate, or adds the label with a user token, so the primary requested workflow leaves branch protection blocked.

Useful? React with 👍 / 👎.

Then wait for the hosted generator checks before merging.
MSG
exit 1
fi

@claude claude Bot Jun 20, 2026

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.

The file ends without an explicit exit 0. This is valid bash — the implicit exit code is the last command's result, and here the heredoc cat in the failure branch returns 0, so non-error paths are fine. Adding exit 0 as the final line would make the intent self-documenting and protect against a future edit that appends a failing command above this point.

Suggested change
fi
exit 1
fi
exit 0


@claude claude Bot Jun 21, 2026

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.

This exit 0 is unreachable — with set -euo pipefail a bash script exits 0 when it falls off the end normally. Not a bug, but removing it would avoid implying there's a third exit path here.

@claude claude Bot Jun 21, 2026

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.

Redundant with set -euo pipefail — reaching this line already means success. Safe to remove.

Suggested change

exit 0

@claude claude Bot Jun 21, 2026

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.

Redundant — set -euo pipefail means the script already exits 0 after the last successful statement. Safe to drop.

Suggested change
exit 0

113 changes: 113 additions & 0 deletions script/ci-required-hosted-gate-test.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
#!/usr/bin/env bash
# Test harness for script/ci-required-hosted-gate.

set -uo pipefail

@claude claude Bot Jun 21, 2026

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.

Missing -e compared to the gate script's set -euo pipefail. Any error outside of a run_case block (e.g. a bad path substitution) will silently continue instead of aborting the test run. The local set +e / set -e inside run_case still handles the gate invocation correctly even with -e at the top level.

Suggested change
set -uo pipefail
set -euo pipefail

@claude claude Bot Jun 21, 2026

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.

-e is absent here but gets enabled inside run_case via set -e after each test execution. This means exit-on-error is off during the first test's outer-script context and on for all subsequent ones. Adding -e here and relying on the existing set +e/set -e guards in run_case makes the behaviour consistent throughout.

Suggested change
set -uo pipefail
set -euo pipefail


SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
GATE="$SCRIPT_DIR/ci-required-hosted-gate"

TESTS_RUN=0
TESTS_FAILED=0
FAILURES=()

fail() {
local message="$1"
TESTS_FAILED=$((TESTS_FAILED + 1))
FAILURES+=("$message")
echo " FAIL: $message" >&2
}

assert_contains() {
local haystack="$1"
local needle="$2"
local label="$3"

case "$haystack" in
*"$needle"*) ;;
*) fail "$label: expected '$needle' in '$haystack'" ;;
esac
}

run_case() {
local name="$1"
local expected_rc="$2"
local event_name="$3"
local run_generators="$4"
local should_run_hosted_ci="$5"
local expected_output="${6:-}"

TESTS_RUN=$((TESTS_RUN + 1))
echo "-> $name"

local output rc
set +e
output="$(
GITHUB_EVENT_NAME="$event_name" \
RUN_GENERATORS="$run_generators" \
SHOULD_RUN_HOSTED_CI="$should_run_hosted_ci" \
"$GATE" 2>&1
)"
rc=$?
set -e

if [ "$rc" -ne "$expected_rc" ]; then
fail "$name: expected exit $expected_rc, got $rc; output: $output"
fi

if [ -n "$expected_output" ]; then
assert_contains "$output" "$expected_output" "$name"
fi
}

run_case \
"blocks generator pull requests without hosted CI" \
1 \
"pull_request" \
"true" \
"false" \
"Generator changes require hosted CI before merge."

run_case \
"allows generator pull requests once hosted CI is requested" \
0 \
"pull_request" \
"true" \
"true"

run_case \
"allows ordinary pull requests without hosted CI" \
0 \
"pull_request" \
"false" \
"false"

run_case \
"allows merge queue generator checks" \
0 \
"merge_group" \
Comment on lines +80 to +87

@claude claude Bot Jun 20, 2026

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.

The merge queue test passes should_run_hosted_ci="true", but the gate's outer condition is [ "$event_name" = "pull_request" ], so the should_run_hosted_ci value is never read for non-PR events. This test passes even if you change the last argument to "false", meaning it doesn't exercise what it appears to document.

Consider adding a companion case that makes the real invariant explicit:

Suggested change
"pull_request" \
"false" \
"false"
run_case \
"allows merge queue generator checks" \
0 \
"merge_group" \
run_case \
"allows merge queue generator checks" \
0 \
"merge_group" \
"true" \
"true"
run_case \
"allows merge queue even without hosted CI flag (event_name guard is sufficient)" \
0 \
"merge_group" \
"true" \
"false"

"true" \
"true"

run_case \
"allows merge queue generator checks even without hosted PR state" \
0 \
"merge_group" \
"true" \
"false"

run_case \

@claude claude Bot Jun 21, 2026

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.

This combination (merge_group + SHOULD_RUN_HOSTED_CI=false) can't happen in production: .github/actions/hosted-ci-selectors always emits should_run_hosted_ci=true for merge_group events because eventName !== 'pull_request'. The test is valid as a gate-in-isolation unit case, but worth a comment so readers don't think this reflects a real workflow state.

"allows non pull request events with unset event name" \
0 \
"" \
"true" \
"false"

if [ "$TESTS_FAILED" -ne 0 ]; then
echo
echo "$TESTS_FAILED of $TESTS_RUN tests failed"
printf ' - %s\n' "${FAILURES[@]}"
exit 1
fi

echo
echo "$TESTS_RUN tests passed"

@claude claude Bot Jun 20, 2026

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.

There's no test for an empty or unrecognised GITHUB_EVENT_NAME (e.g. workflow_dispatch, schedule, or a locally sourced run where the var is unset). The gate defaults event_name to "" and correctly passes, but that path has no coverage. A case like:

run_case \
  "allows unrecognised event names (e.g. workflow_dispatch)" \
  0 \
  "workflow_dispatch" \
  "true" \
  "false"

would pin this and catch any future edit that widens the outer if condition.

@claude claude Bot Jun 21, 2026

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.

The PR description's Validation section says "4 tests passed", but there are 6 run_case calls in this file. The code is correct — just a stale count in the PR body.

Loading