ci: introduce deterministic native merge queue gate + PR queue visibility#22293
ci: introduce deterministic native merge queue gate + PR queue visibility#22293BrianCLong wants to merge 1 commit intomainfrom
Conversation
BrianCLong
added
the
codex
ChatGPT Codex Connector
WalkthroughGitHub Actions workflows transitioned from scheduled automation to manual/event-driven triggers. Merge queue feeder and train autopilot cron jobs were deactivated in favor of pull request events. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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: 337812e8b7
ℹ️ 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".
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
| group: merge-queue-main | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
merge-queue.yml now triggers on high-frequency pull_request actions (synchronize, reopened, etc.) while using a single workflow-level concurrency lock (merge-queue-main) with cancel-in-progress: false. Since workflow concurrency is shared across all runs of this workflow, comment-only queue-visibility runs can backlog ahead of merge-queue-critical, delaying the required merge-queue validation and stalling merges during routine PR activity. Scope concurrency to the merge_group path (or split groups by event) so required queue checks are not blocked by visibility updates.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the branch protection configuration for the main branch by adding 'merge-queue / merge-queue-critical' to the required status checks and adjusting the character encoding for the 'CI Core Gate' entry. Feedback highlights a critical concern regarding a potential deadlock in the merge queue if existing required checks are not triggered by the merge_group event, and suggests removing redundant checks to ensure the queue can progress.
| "test (20.x)", | ||
| "Workflow Validity Check" | ||
| "Workflow Validity Check", | ||
| "merge-queue / merge-queue-critical" |
There was a problem hiding this comment.
Retaining existing required checks (e.g., Unit Tests, gate, CI Core Gate) alongside the new merge-queue / merge-queue-critical check is likely to cause a deadlock in the merge queue.
According to the PR description, the automation workflows providing these checks have been converted to manual workflow_dispatch. If these checks are not configured to trigger on the merge_group event, they will never report a status for merge candidates, causing the queue to wait indefinitely.
Furthermore, if merge-queue-critical already performs install, lint, typecheck, and test, these existing checks are redundant. You should remove the redundant or disabled checks from this list to ensure the merge queue can progress.
Additionally, please ensure the check name merge-queue / merge-queue-critical exactly matches the status reported by the new workflow (this requires the workflow in .github/workflows/merge-queue.yml to have name: merge-queue).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/merge-train-autopilot.yml (1)
19-33:⚠️ Potential issue | 🟠 MajorNo-op autopilot should not request write scope or perform checkout.
Current permissions and checkout are unnecessary for the remaining behavior and increase risk/cost without value.
🧹 Proposed hardening + simplification
permissions: contents: read - pull-requests: write @@ steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - fetch-tags: true - - name: Run Autopilot run: | echo "Autopilot now manual-only to prevent unsupervised queue churn." echo "Use GitHub native merge queue for routine sequencing."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/merge-train-autopilot.yml around lines 19 - 33, The workflow grants unnecessary write scope and performs a repository checkout for the no-op autopilot job; remove the "pull-requests: write" permission from the permissions block and delete the actions/checkout step (including its with: fetch-depth/fetch-tags keys) in the "autopilot" job so the job only uses minimal read permissions (contents: read) and does not perform a checkout; locate the permissions block and the "uses: actions/checkout@v4" step to apply these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/merge-queue-feeder.yml:
- Around line 6-8: The workflow grants unnecessary write scope via the
permissions key "pull-requests: write"; remove the "pull-requests: write" entry
under permissions so the permissions block only includes "contents: read" (or
explicitly set pull-requests: none) to follow least-privilege; check that no
steps in the workflow use PR mutation APIs (references to pull-requests) before
committing the change, and update any code that required write access if found.
In @.github/workflows/merge-queue.yml:
- Around line 127-129: The merge-queue job currently runs "pnpm run lint
--if-present", "pnpm run typecheck --if-present", and "pnpm run test
--if-present" which allows silent success when scripts are missing; remove the
"--if-present" flags so the commands always fail if the corresponding npm
scripts are absent (update the three invocations for lint, typecheck and test),
and optionally add an explicit pre-check that validates those scripts exist in
package.json or restore the missing scripts if needed to ensure the gate always
executes real validations.
- Around line 14-16: The workflow-level concurrency block should be removed and
instead added to the merge-queue-critical job only: delete the top-level
concurrency stanza and add a concurrency: group: merge-queue-main
cancel-in-progress: false block inside the job named merge-queue-critical so
only that job is serialized (leaving pull_request/queue-visibility runs
unconstrained); update the workflow YAML accordingly to move the concurrency
constraint from the workflow root to the merge-queue-critical job definition.
- Around line 75-98: Wrap the block that calls
github.paginate(github.rest.issues.listComments) and the subsequent
github.rest.issues.updateComment/createComment calls in a try-catch so
permission errors from forked PRs do not fail the workflow; catch any error
thrown around the comments lookup/update/create (the variables/comments flow
that computes existing from comments, and calls updateComment or createComment
with body and marker) and log a warning including the error message (instead of
throwing), then continue silently for visibility-only runs.
---
Outside diff comments:
In @.github/workflows/merge-train-autopilot.yml:
- Around line 19-33: The workflow grants unnecessary write scope and performs a
repository checkout for the no-op autopilot job; remove the "pull-requests:
write" permission from the permissions block and delete the actions/checkout
step (including its with: fetch-depth/fetch-tags keys) in the "autopilot" job so
the job only uses minimal read permissions (contents: read) and does not perform
a checkout; locate the permissions block and the "uses: actions/checkout@v4"
step to apply these changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdc48ac6-c344-4983-8dda-74264e8ee656
📒 Files selected for processing (5)
.github/protection/main.required-checks.json.github/workflows/merge-group-heavy.yml.github/workflows/merge-queue-feeder.yml.github/workflows/merge-queue.yml.github/workflows/merge-train-autopilot.yml
| permissions: | ||
| FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true | ||
| contents: write | ||
| contents: read | ||
| pull-requests: write |
There was a problem hiding this comment.
Drop unnecessary pull-requests: write permission for a no-op workflow.
This workflow no longer performs PR mutations, so retaining write scope is excess privilege.
🔐 Proposed least-privilege fix
permissions:
contents: read
- pull-requests: write📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: | |
| FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true | |
| contents: write | |
| contents: read | |
| pull-requests: write | |
| permissions: | |
| contents: read |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/merge-queue-feeder.yml around lines 6 - 8, The workflow
grants unnecessary write scope via the permissions key "pull-requests: write";
remove the "pull-requests: write" entry under permissions so the permissions
block only includes "contents: read" (or explicitly set pull-requests: none) to
follow least-privilege; check that no steps in the workflow use PR mutation APIs
(references to pull-requests) before committing the change, and update any code
that required write access if found.
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
| group: merge-queue-main | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GitHub Actions, does workflow-level concurrency apply to all runs regardless of which job executes, and can it serialize runs across different event types in the same workflow?
💡 Result:
Yes, workflow-level concurrency in GitHub Actions applies to all runs of that workflow regardless of which job executes, and it can serialize runs across different event types in the same workflow.
Citations:
- 1: https://docs.github.com/actions/writing-workflows/choosing-what-your-workflow-does/control-the-concurrency-of-workflows-and-jobs
- 2: https://docs.github.com/en/actions/using-jobs/using-concurrency
- 3: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/control-the-concurrency-of-workflows-and-jobs
🏁 Script executed:
cat -n .github/workflows/merge-queue.ymlRepository: BrianCLong/summit
Length of output: 5369
Move concurrency constraint to merge-queue-critical job only.
Workflow-level concurrency serializes both pull_request runs (queue-visibility) and merge_group runs (merge-queue-critical). This queues informational PR visibility updates ahead of required merge gate validations, reducing merge throughput.
Fix: Apply concurrency to the critical validation job only
-concurrency:
- group: merge-queue-main
- cancel-in-progress: false
-
jobs:
queue-visibility:
if: github.event_name == 'pull_request'
runs-on: ubuntu-latest
merge-queue-critical:
if: github.event_name == 'merge_group'
+ concurrency:
+ group: merge-queue-main
+ cancel-in-progress: false
runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/merge-queue.yml around lines 14 - 16, The workflow-level
concurrency block should be removed and instead added to the
merge-queue-critical job only: delete the top-level concurrency stanza and add a
concurrency: group: merge-queue-main cancel-in-progress: false block inside the
job named merge-queue-critical so only that job is serialized (leaving
pull_request/queue-visibility runs unconstrained); update the workflow YAML
accordingly to move the concurrency constraint from the workflow root to the
merge-queue-critical job definition.
| const comments = await github.paginate(github.rest.issues.listComments, { | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: pr.number, | ||
| per_page: 100, | ||
| }); | ||
|
|
||
| const existing = comments.find((comment) => comment.body?.includes(marker)); | ||
|
|
||
| if (existing) { | ||
| await github.rest.issues.updateComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| comment_id: existing.id, | ||
| body, | ||
| }); | ||
| } else { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: pr.number, | ||
| body, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GitHub Actions pull_requestevents from forks, can workflow-levelpull-requests: writestill be downgraded soissues.createComment/issues.updateComment fails with permission errors?
💡 Result:
Yes, for GitHub Actions workflows triggered by pull_request events from forks (excluding pull_request_target), the GITHUB_TOKEN permissions are automatically downgraded to read-only if the repository setting "Send write tokens to workflows from pull requests" is not enabled. This downgrade occurs even if the workflow explicitly requests pull-requests: write permissions. As a result, API calls like issues.createComment or issues.updateComment (which require issues: write or pull-requests: write) will fail with permission errors such as "Resource not accessible by integration" (HTTP 403). This security measure prevents untrusted fork workflows from performing write operations on the base repository. The official documentation confirms: "if the workflow was triggered by a pull request event other than pull_request_target from a forked repository, and the Send write tokens to workflows from pull requests setting is not selected, the permissions are adjusted to change any write permissions to read only." To allow write permissions: - Enable the "Send write tokens to workflows from pull requests" setting in repository Settings > Actions > General (available for private repositories; public repos have additional restrictions). - Or use pull_request_target trigger (runs workflow from base branch with full permissions, but requires caution against malicious PR code). Note: issues.createComment maps to issues: write permission, and PR comments also work with pull-requests: write. Both are downgraded in this scenario.
Citations:
- 1: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions
- 2: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
- 3: Support review of pull requests created from forks google-github-actions/run-gemini-cli#191
- 4: Unable to Comment on PR's using Github Actions cli/cli#10464
- 5: https://shreyapohekar.com/blogs/the-right-way-to-handle-permissions-in-github-actions-a-practical-guide-to-staying-secure/
🏁 Script executed:
# Check if the file exists and read the relevant section
fd -t f 'merge-queue.yml' --exec cat -n {} \;Repository: BrianCLong/summit
Length of output: 43
🏁 Script executed:
# List files in .github/workflows directory
ls -la .github/workflows/ 2>/dev/null || echo "Directory not found"
# Search for any files with 'merge' in name
fd -t f 'merge'
# Search for the specific code pattern to find where it lives
rg "listComments" -t yaml -A 5Repository: BrianCLong/summit
Length of output: 50373
🏁 Script executed:
# Read the merge-queue.yml file with line numbers
cat -n .github/workflows/merge-queue.yml | head -120Repository: BrianCLong/summit
Length of output: 4571
Wrap comment operations in try-catch to handle fork PR permission restrictions.
Lines 75-98 lack error handling for listComments, createComment, and updateComment. On pull_request events from forks, the GITHUB_TOKEN is automatically downgraded to read-only (even with explicit pull-requests: write permission), causing these operations to fail with 403 errors. Since this is a visibility-only job, permission failures should be logged as warnings, not fail the workflow.
🛡️ Proposed fail-soft handling
+ try {
const comments = await github.paginate(github.rest.issues.listComments, {
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pr.number,
per_page: 100,
});
const existing = comments.find((comment) => comment.body?.includes(marker));
if (existing) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: existing.id,
body,
});
} else {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pr.number,
body,
});
}
+ } catch (error) {
+ core.warning(`Unable to publish queue status comment: ${error.message}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const comments = await github.paginate(github.rest.issues.listComments, { | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: pr.number, | |
| per_page: 100, | |
| }); | |
| const existing = comments.find((comment) => comment.body?.includes(marker)); | |
| if (existing) { | |
| await github.rest.issues.updateComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| comment_id: existing.id, | |
| body, | |
| }); | |
| } else { | |
| await github.rest.issues.createComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: pr.number, | |
| body, | |
| }); | |
| } | |
| try { | |
| const comments = await github.paginate(github.rest.issues.listComments, { | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: pr.number, | |
| per_page: 100, | |
| }); | |
| const existing = comments.find((comment) => comment.body?.includes(marker)); | |
| if (existing) { | |
| await github.rest.issues.updateComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| comment_id: existing.id, | |
| body, | |
| }); | |
| } else { | |
| await github.rest.issues.createComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: pr.number, | |
| body, | |
| }); | |
| } | |
| } catch (error) { | |
| core.warning(`Unable to publish queue status comment: ${error.message}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/merge-queue.yml around lines 75 - 98, Wrap the block that
calls github.paginate(github.rest.issues.listComments) and the subsequent
github.rest.issues.updateComment/createComment calls in a try-catch so
permission errors from forked PRs do not fail the workflow; catch any error
thrown around the comments lookup/update/create (the variables/comments flow
that computes existing from comments, and calls updateComment or createComment
with body and marker) and log a warning including the error message (instead of
throwing), then continue silently for visibility-only runs.
| pnpm run lint --if-present | ||
| pnpm run typecheck --if-present | ||
| pnpm run test --if-present |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does pnpm run <script> --if-present exit successfully when the script is missing, and therefore skip the check without failing CI?
💡 Result:
Yes, pnpm run <script> --if-present exits successfully (exit code 0) when the script is missing. It skips the script without failing, allowing the command to succeed in CI pipelines.
Citations:
- 1: https://pnpm.io/cli/run
- 2: https://pnpm.io/9.x/cli/run
- 3: pnpm run-script no longer supports --if-present flag pnpm/pnpm#2200
- 4: https://kollitsch.dev/blog/2023/how-to-run-npm-scripts-without-issues-when-they-dont-exist
- 5: https://nesin.io/blog/npm-script-if-present
- 6: https://stackoverflow.com/questions/50683885/how-to-check-if-npm-script-exists
🏁 Script executed:
# Find and examine the merge-queue.yml file
fd -t f "merge-queue.yml" .github/workflows/Repository: BrianCLong/summit
Length of output: 94
🏁 Script executed:
# Read the merge-queue.yml file to see the context around lines 127-129
cat -n .github/workflows/merge-queue.yml | head -150 | tail -50Repository: BrianCLong/summit
Length of output: 1920
🏁 Script executed:
# Check if package.json exists and see if lint, typecheck, test scripts are defined
if [ -f package.json ]; then
echo "=== Checking package.json for scripts ==="
cat package.json | jq '.scripts | keys[] | select(. == "lint" or . == "typecheck" or . == "test")'
fiRepository: BrianCLong/summit
Length of output: 128
Remove --if-present from critical validation suite in merge-queue gate.
--if-present silently succeeds when scripts are missing, which can produce a passing required check without actually running lint, typecheck, or test. For a merge-queue gate, these validations must always execute. While the scripts currently exist in package.json, using --if-present creates risk if future refactoring accidentally removes one.
Proposed fix
- name: Critical validation suite
run: |
- pnpm run lint --if-present
- pnpm run typecheck --if-present
- pnpm run test --if-present
+ pnpm run lint
+ pnpm run typecheck
+ pnpm run test📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pnpm run lint --if-present | |
| pnpm run typecheck --if-present | |
| pnpm run test --if-present | |
| pnpm run lint | |
| pnpm run typecheck | |
| pnpm run test |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/merge-queue.yml around lines 127 - 129, The merge-queue
job currently runs "pnpm run lint --if-present", "pnpm run typecheck
--if-present", and "pnpm run test --if-present" which allows silent success when
scripts are missing; remove the "--if-present" flags so the commands always fail
if the corresponding npm scripts are absent (update the three invocations for
lint, typecheck and test), and optionally add an explicit pre-check that
validates those scripts exist in package.json or restore the missing scripts if
needed to ensure the gate always executes real validations.
|
Closing duplicate — superseded by #22294 |
1 participant
Motivation
main.Description
.github/workflows/merge-queue.ymlthat:merge_groupforchecks_requestedand topull_requestqueue lifecycle events, publishes a persistent PR comment with queue position/depth/reason, and runs an integrated validation jobmerge-queue-criticalthat performsinstall,lint,typecheck, andteston the merge candidate.merge-queue-main) withcancel-in-progress: falseto serialize queue validations and avoid superseding in-flight validations..github/workflows/merge-group-heavy.yml,.github/workflows/merge-queue-feeder.yml, and.github/workflows/merge-train-autopilot.ymltoworkflow_dispatch/no-op operator guidance..github/protection/main.required-checks.jsonasmerge-queue / merge-queue-criticalso the integrated validation is required before main merges.Testing
ruby -ryamland all modified workflow files loaded successfully (ok .github/workflows/*.yml). — Success..github/protection/main.required-checks.jsonwithjqto ensure valid JSON structure. — Success.workflow_dispatchormerge_grouptriggers as intended. — Success.Codex Task
Summary by CodeRabbit
New Features
Chores