Require hosted CI for generator PRs#4149
Conversation
WalkthroughIntroduces ChangesGenerator-sensitive hosted CI gate
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Post-merge workflow exercise follow-up created: #4150 |
|
+ci-force-full |
github-actions
Bot
added
force-full-hosted-ci
Force-Full Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
Code ReviewSummaryThis PR adds a lightweight guard to the required CI gate that blocks generator-sensitive PRs from merging until hosted CI is explicitly requested. The approach is clean and targeted: it extends the existing What works well
IssuesTest coverage gap – merge queue case is under-specified Test case 4 passes run_case "allows merge queue generator checks" 0 "merge_group" "true" "true"The gate's outer condition is Missing test: unknown/empty event name Scheduled runs, Minor observations
VerdictThe logic is sound and the implementation is clean. The two test-coverage gaps above are worth addressing — especially the merge queue case, which currently does not exercise what it appears to test — but neither is a blocking correctness issue. Overall this is a solid, low-risk addition that directly addresses the gap described in the PR. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "pull_request" \ | ||
| "false" \ | ||
| "false" | ||
|
|
||
| run_case \ | ||
| "allows merge queue generator checks" \ | ||
| 0 \ | ||
| "merge_group" \ |
There was a problem hiding this comment.
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:
| "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" |
| fi | ||
|
|
||
| echo | ||
| echo "$TESTS_RUN tests passed" |
There was a problem hiding this comment.
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.
| Then wait for the hosted generator checks before merging. | ||
| MSG | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
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.
| fi | |
| exit 1 | |
| fi | |
| exit 0 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3146b5f2-2272-4778-9131-4f9063a5e8c3) |
|
+ci-force-full |
Force-Full Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
justin808
force-pushed
the
jg-codex/require-hosted-ci-for-generators
branch
from
4bb8428 to
d7e279c
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bb8428998
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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. |
There was a problem hiding this comment.
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 👍 / 👎.
Code ReviewOverviewThis PR closes a real process gap: generator-touching PRs could previously merge after only the lightweight What's Well Done
IssuesTest count mismatch in PR description. The Validation section says "4 tests passed" but the test harness actually has 6 Redundant SecurityNo concerns. All env vars consumed by the gate script are set by the trusted GitHub Actions environment or the workflow's own VerdictSolid, well-scoped fix. The two items above are both minor. Approving. |
| fi | ||
|
|
||
| echo | ||
| echo "$TESTS_RUN tests passed" |
There was a problem hiding this comment.
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.
| MSG | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
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.
Code Review — PR #4149: Require hosted CI for generator PRsOverall: Well-targeted fix that closes a real process gap (see #4130). The gate script is small, deterministic, and correctly scoped to only Correctness
Minor Issues
Not Issues (confirming correctness)
|
| MSG | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
Redundant with set -euo pipefail — reaching this line already means success. Safe to remove.
| #!/usr/bin/env bash | ||
| # Test harness for script/ci-required-hosted-gate. | ||
|
|
||
| set -uo pipefail |
There was a problem hiding this comment.
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.
| set -uo pipefail | |
| set -euo pipefail |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryCloses the gap where a generator-sensitive PR could pass the required branch-protection gate (
Confidence Score: 4/5Safe to merge; the gate script is minimal and its worst-case failure mode is blocking a PR that should be allowed, not allowing one that should be blocked. The gate logic is correct and well-tested across six cases. The
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ci-required.yml triggered] --> B{Event type?}
B -->|pull_request| C[hosted-ci-selectors\nreads ready-for-hosted-ci label\noutputs should_run_hosted_ci]
B -->|merge_group / workflow_dispatch / push| G[Gate: event_name ≠ pull_request\n→ exit 0 ✅]
C --> D[ci-changes-detector\noutputs run_generators]
D --> E{run_generators = true\nAND should_run_hosted_ci ≠ true?}
E -->|YES| F[exit 1 ❌\nRequired gate fails\nContributor must add ready-for-hosted-ci]
E -->|NO| H[exit 0 ✅\nRequired gate passes]
%%{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
A[ci-required.yml triggered] --> B{Event type?}
B -->|pull_request| C[hosted-ci-selectors\nreads ready-for-hosted-ci label\noutputs should_run_hosted_ci]
B -->|merge_group / workflow_dispatch / push| G[Gate: event_name ≠ pull_request\n→ exit 0 ✅]
C --> D[ci-changes-detector\noutputs run_generators]
D --> E{run_generators = true\nAND should_run_hosted_ci ≠ true?}
E -->|YES| F[exit 1 ❌\nRequired gate fails\nContributor must add ready-for-hosted-ci]
E -->|NO| H[exit 0 ✅\nRequired gate passes]
Reviews (1): Last reviewed commit: "Cover hosted CI gate edge cases" | Re-trigger Greptile |
Code Review — PR #4149: Require hosted CI for generator PRsOverviewClean, focused change that closes a real gap. PR #4130's post-merge failure in the generator shard is a solid motivating case, and the implementation correctly targets only the lightweight required gate without affecting merge-queue or push-to-main paths. CorrectnessThe gate logic in
IssuesMinor — redundant Minor — Cosmetic — stale test count in PR description Observation — one test exercises a scenario that can't happen in production Positive notes
|
| exit 1 | ||
| fi | ||
|
|
||
| exit 0 |
There was a problem hiding this comment.
Redundant — set -euo pipefail means the script already exits 0 after the last successful statement. Safe to drop.
| exit 0 |
| #!/usr/bin/env bash | ||
| # Test harness for script/ci-required-hosted-gate. | ||
|
|
||
| set -uo pipefail |
There was a problem hiding this comment.
-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.
| set -uo pipefail | |
| set -euo pipefail |
| "true" \ | ||
| "false" | ||
|
|
||
| run_case \ |
There was a problem hiding this comment.
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.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d5664f52-d882-4070-bd67-999aaf4373b8) |
size-limit report 📦
|
Code Review: Require hosted CI for generator PRsOverviewThis PR introduces a lightweight CI guard that prevents generator-sensitive PRs from passing CorrectnessThe gate logic is correct:
Minor Issues1. Redundant With 2. Missing test cases for The test harness covers Suggested additions: run_case \
"allows workflow_dispatch events even with generator changes" \
0 \
"workflow_dispatch" \
"true" \
"false"
run_case \
"allows push events even with generator changes" \
0 \
"push" \
"true" \
"false"3. Unrelated test fix bundled into CI infrastructure PR
Positive Notes
SummaryThe implementation is correct and the approach is sound. The three items above are all minor — the redundant |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_be2aaba2-ee25-497f-baaf-897836008e29) |
|
Confidence note:
|
…-pr-batch-skills * origin/main: Bound the RSCProvider RSC payload cache with an LRU (#3564) (#4097) Harden PR security preflight follow-ups (#4151) [codex] Trust ShakaCode team for agent inputs (#4154) Default adversarial PR review to current branch (#4155) Require hosted CI for generator PRs (#4149) # Conflicts: # .agents/skills/pr-batch/SKILL.md # .agents/workflows/pr-processing.md
…trap-4138 * origin/main: Docs: point Pro license to purchase site (#4160) docs: paired ShakaPerf A/B methodology for RSC perf regressions (#4137) (#4142) Bound the RSCProvider RSC payload cache with an LRU (#3564) (#4097) Harden PR security preflight follow-ups (#4151) [codex] Trust ShakaCode team for agent inputs (#4154) Default adversarial PR review to current branch (#4155) Require hosted CI for generator PRs (#4149) Make review-fix push confirmation context-sensitive (#4152) Harden PR batch security preflight (#4148) Add bidirectional streaming for async props (pull mode) (#4048) Fix main generator pretend spec isolation (#4147) Revive Pro streaming validation coverage (#4035) Fix ci-local detector JSON parsing (#4061) ci(pro): add Rspack build leg for execjs-compatible-dummy (#4091) (#4106) Add High-Risk Mode to adversarial-pr-review (#4050) (#4115) Fix hydration error reporting for thrown values (#4120) Add batch cancellation / drain protocol to stop in-flight PR batches (#4119) [codex] Document current RSC architecture and version policy (#4103) Clarify PR-batch merge authority and ready gates (#4140) # Conflicts: # docs/oss/building-features/performance-tracks-and-profiling.md # llms-full.txt
…o-agent-guidance * origin/main: (647 commits) Consolidate analysis/ into internal/analysis/ (#4159) Docs: point Pro license to purchase site (#4160) docs: paired ShakaPerf A/B methodology for RSC perf regressions (#4137) (#4142) Bound the RSCProvider RSC payload cache with an LRU (#3564) (#4097) Harden PR security preflight follow-ups (#4151) [codex] Trust ShakaCode team for agent inputs (#4154) Default adversarial PR review to current branch (#4155) Require hosted CI for generator PRs (#4149) Make review-fix push confirmation context-sensitive (#4152) Harden PR batch security preflight (#4148) Add bidirectional streaming for async props (pull mode) (#4048) Fix main generator pretend spec isolation (#4147) Revive Pro streaming validation coverage (#4035) Fix ci-local detector JSON parsing (#4061) ci(pro): add Rspack build leg for execjs-compatible-dummy (#4091) (#4106) Add High-Risk Mode to adversarial-pr-review (#4050) (#4115) Fix hydration error reporting for thrown values (#4120) Add batch cancellation / drain protocol to stop in-flight PR batches (#4119) [codex] Document current RSC architecture and version policy (#4103) Clarify PR-batch merge authority and ready gates (#4140) ... # Conflicts: # AGENTS_USER_GUIDE.md
…ales-js * origin/main: [codex] Document flagship demo coordination (#4164) Enrich deferred-render RSC errors with the bundle diagnostic (#3475) (#4100) Consolidate analysis/ into internal/analysis/ (#4159) Docs: point Pro license to purchase site (#4160) docs: paired ShakaPerf A/B methodology for RSC perf regressions (#4137) (#4142) Bound the RSCProvider RSC payload cache with an LRU (#3564) (#4097) Harden PR security preflight follow-ups (#4151) [codex] Trust ShakaCode team for agent inputs (#4154) Default adversarial PR review to current branch (#4155) Require hosted CI for generator PRs (#4149) Make review-fix push confirmation context-sensitive (#4152) Harden PR batch security preflight (#4148) Add bidirectional streaming for async props (pull mode) (#4048) Fix main generator pretend spec isolation (#4147)
1 participant
Summary
Add a lightweight
ci-requiredguard so ordinary pull requests that touch generator-sensitive paths cannot pass the required gate until hosted CI is requested.Why
PR #4130 changed generator path behavior and had
ci-required / required-pr-gategreen, butRspec test for gem / rspec-package-testswas skipped on the PR. The generator shard then failed only after merge tomain.script/ci-changes-detectoralready knows generator changes need generator/Ruby coverage. This PR makes the fast required gate enforce the missing process step: request hosted CI before merging generator-sensitive PRs.Implementation
script/ci-required-hosted-gateand a shell test harness..github/workflows/ci-required.ymlto:.github/actions/hosted-ci-selectorsrun_generators=trueand hosted CI is not requestedAGENTS.md,.github/read-me.md, andinternal/contributor-info/ci-optimization.md.Validation
bash script/ci-required-hosted-gate-test.bash- 6 tests passedbash -n script/ci-required-hosted-gate script/ci-required-hosted-gate-test.bash script/ci-changes-detector- passedruby -e "require 'yaml'; Dir['.github/workflows/*.{yml,yaml}'].sort.each { |f| YAML.safe_load_file(f, permitted_classes: [], aliases: false) }; puts 'workflow YAML OK'"- passedbash script/ci-changes-detector-test.bash- 49 tests passedactionlint .github/workflows/ci-required.yml- passedpnpm dlx prettier@3.6.2 --check AGENTS.md .github/read-me.md internal/contributor-info/ci-optimization.md .github/workflows/ci-required.yml- passedpnpm install --frozen-lockfile- passed; no tracked install churnpnpm start format.listDifferent- passed after installing workspace dependenciesProcess Gap Disposition
Mechanism target:
script.Motivating miss: generator-sensitive PRs could merge after only
ci-required / required-pr-gate, even when the hosted generator shard that would catch order/state failures was skipped.Replay evidence: #4130 had a green required gate and skipped
rspec-package-tests; post-mergemainfailed in the generator shard. #4147 fixed the spec failure. This PR blocks the equivalent future PR state before merge.Non-goal: require full hosted CI for every PR. The guard only applies when change detection marks generator coverage relevant.
Labels
Labels: ready-for-hosted-ci, force-full-hosted-ci- changes GitHub Actions required-gate behavior, so path/hosted-CI selection itself is part of the risk.Follow-up
Because this changes
.github/workflows/ci-required.yml, follow-up issue #4150 was created to exercise the post-merge behavior: #4150.Note
Medium Risk
Changes branch-protection behavior and CI routing for generator PRs; incorrect gating could block merges or allow skips, though behavior is covered by shell tests and documented exceptions for merge queue/release targets.
Overview
Ordinary PRs that touch generator-sensitive paths can no longer pass
ci-required / required-pr-gateuntil hosted CI is requested. A newscript/ci-required-hosted-gatefails when the event ispull_request,script/ci-changes-detectorsetsrun_generators=true, andhosted-ci-selectorsreports hosted CI is not eligible (e.g. noready-for-hosted-ci). Merge queue and other non-PR paths are unchanged.ci-required.ymlnow runshosted-ci-selectors, captures detector outputs on the changes step, invokes the gate, addsissues: read, validates the new script, and runsci-required-hosted-gate-test.bashin the required job.Contributor docs (
AGENTS.md,.github/read-me.md,ci-optimization.md) describe the generator exception. Pro node renderer:incrementalHtmlStreaming.test.tsupdates the expected async-prop error message string only.Reviewed by Cursor Bugbot for commit 7228de8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit