feat: external contributor PR review gate#2740
Conversation
…aintainer PRs External PRs from non-collaborators now get a review-request comment and 'external-contributor' label instead of auto-merge. The pulse checks author permission via gh api before any merge decision. Workers are also blocked from fixing CI on external PRs — that's the contributor's job. Added as Hard Rule 12 to ensure compliance across all pulse sessions.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical security enhancement to the automated PR merging process. It addresses a vulnerability where PRs from external contributors could potentially be auto-merged without human review, as identified by a previous incident. The changes ensure that all external contributions are explicitly flagged for maintainer review, preventing unauthorized or problematic code from entering the repository automatically and reinforcing the repository's security posture. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a mandatory external-contributor verification gate and a new hard rule preventing auto-merge for external authors or permission-check failures; queries GitHub for author permissions, idempotently labels/comments external PRs, and skips automatic worker/CI dispatch for those PRs. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant Gate as External Contributor Gate
participant API as GitHub API
participant Merge as Merge Handler
participant Maint as Maintainer
PR->>Gate: PR received
Gate->>API: Query author collaborator permission
API-->>Gate: Return permission level / error
alt Maintainer (admin/maintain/write)
Gate->>Merge: Allow normal merge flow
Merge->>Merge: Run CI/workers & evaluate reviews
Merge->>PR: Auto-merge if conditions satisfied
else External Contributor (read/none/404)
Gate->>PR: Add `external-contributor` label (idempotent)
Gate->>PR: Post maintainer-review request comment (idempotent)
Gate->>Merge: Skip automatic worker/CI dispatch
Maint->>PR: Manual review and merge decision
else Permission Check Failure (403/429/5xx)
Gate->>PR: Post manual-intervention comment (idempotent)
Gate->>Merge: Fail-closed — do not label, do not auto-merge, retry later
Maint->>PR: Investigate and act manually
end
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 3 00:02:06 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![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 introduces an important security measure to prevent the automatic merging of pull requests from external contributors. The implementation is well-documented in pulse.md. My review focuses on improving the robustness of the shell commands by removing error suppression (2>/dev/null). This change will make the system easier to debug by ensuring that critical errors, such as authentication failures, are not hidden.
Note: Security Review has been skipped due to the limited scope of the PR.
.agents/scripts/commands/pulse.md
Outdated
| **External contributor gate (MANDATORY):** Before merging ANY PR, check if the author is a repo collaborator: | ||
|
|
||
| ```bash | ||
| gh api "repos/<slug>/collaborators/<author>/permission" --jq '.permission' 2>/dev/null |
There was a problem hiding this comment.
The use of 2>/dev/null is discouraged as it suppresses all error messages, making it difficult to debug issues like authentication failures (e.g., an expired GH_TOKEN) or other unexpected API problems. While this command is intended to handle 404 errors for non-collaborators, suppressing stderr completely can mask critical system-level issues, which might be misinterpreted as a PR from an external contributor. It's better to allow stderr to be visible for debugging. The script logic can be adapted to handle expected errors (like 404s) without silencing all other potential problems.
| gh api "repos/<slug>/collaborators/<author>/permission" --jq '.permission' 2>/dev/null | |
| gh api "repos/<slug>/collaborators/<author>/permission" --jq '.permission' |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
.agents/scripts/commands/pulse.md
Outdated
|
|
||
| ```bash | ||
| gh pr comment <number> --repo <slug> --body "This PR is from an external contributor (@<author>). Auto-merge is disabled for external PRs — a maintainer must review and merge manually." | ||
| gh pr edit <number> --repo <slug> --add-label "external-contributor" 2>/dev/null || true |
There was a problem hiding this comment.
Similar to the previous comment, 2>/dev/null should be avoided here. It hides potentially useful error messages, for example if the external-contributor label doesn't exist in the repository or if there's a permission issue. The || true construct already prevents the script from exiting if set -e is enabled, so stderr can be safely displayed for debugging purposes without halting execution.
| gh pr edit <number> --repo <slug> --add-label "external-contributor" 2>/dev/null || true | |
| gh pr edit <number> --repo <slug> --add-label "external-contributor" || true |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.agents/scripts/commands/pulse.md (1)
72-75: Make external-review commenting idempotent to avoid pulse spam.Given the 2-minute pulse cadence, this will post the same comment repeatedly on every cycle for the same PR. Add a guard (existing label/comment marker check) before posting.
Based on learnings: Follow security protocols for all git and DevOps operations.Suggested idempotent pattern
- gh pr comment <number> --repo <slug> --body "This PR is from an external contributor (@<author>). Auto-merge is disabled for external PRs — a maintainer must review and merge manually." - gh pr edit <number> --repo <slug> --add-label "external-contributor" 2>/dev/null || true + gh pr edit <number> --repo <slug> --add-label "external-contributor" 2>/dev/null || true + EXISTING_NOTE=$(gh pr view <number> --repo <slug> --json comments --jq '.comments[].body' 2>/dev/null | grep -F "Auto-merge is disabled for external PRs" || true) + if [ -z "$EXISTING_NOTE" ]; then + gh pr comment <number> --repo <slug> --body "This PR is from an external contributor (@<author>). Auto-merge is disabled for external PRs — a maintainer must review and merge manually." + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/commands/pulse.md around lines 72 - 75, The pulse script currently runs gh pr comment and gh pr edit unconditionally, causing repeated identical comments; update the logic so it first checks for an existing "external-contributor" label or an existing PR comment containing the same marker text and only runs gh pr comment/gh pr edit if neither exists. Specifically, before calling the gh pr comment and gh pr edit commands shown, query the PR labels (e.g., via gh pr view --json labels) and/or comments (gh pr view --json comments or gh api repos/:owner/:repo/issues/:number/comments) to detect the "external-contributor" label or the authored marker text, and skip posting or adding the label when found, otherwise proceed to post the comment and add the label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/commands/pulse.md:
- Around line 67-71: The current permission check using gh api
"repos/<slug>/collaborators/<author>/permission" treats read/none/404 as
external contributors but doesn't distinguish other API failures; update the
logic that inspects the gh API response and exit status (the call to gh api
"repos/<slug>/collaborators/<author>/permission") so that: successful 2xx
responses continue to map admin/maintain/write → maintainer and read/none →
external contributor (no auto-merge), a 404 still maps to external contributor,
and any non-2xx non-404 responses (e.g., 403, 429, 5xx or network/auth errors)
are handled as “permission check failed” by failing closed — do not mark as
external or auto-merge, instead emit a distinct error/PR comment asking for
manual/maintainer intervention and abort the auto-merge flow.
---
Nitpick comments:
In @.agents/scripts/commands/pulse.md:
- Around line 72-75: The pulse script currently runs gh pr comment and gh pr
edit unconditionally, causing repeated identical comments; update the logic so
it first checks for an existing "external-contributor" label or an existing PR
comment containing the same marker text and only runs gh pr comment/gh pr edit
if neither exists. Specifically, before calling the gh pr comment and gh pr edit
commands shown, query the PR labels (e.g., via gh pr view --json labels) and/or
comments (gh pr view --json comments or gh api
repos/:owner/:repo/issues/:number/comments) to detect the "external-contributor"
label or the authored marker text, and skip posting or adding the label when
found, otherwise proceed to post the comment and add the label.
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 3 00:12:15 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.agents/scripts/commands/pulse.md (1)
67-71:⚠️ Potential issue | 🟠 MajorDifferentiate permission-check failures from true external contributors.
Line 67 currently suppresses API errors but the flow text only classifies maintainer vs external. Please explicitly branch on HTTP status/exit code so 403/429/5xx/auth/network failures become “permission check failed” (manual maintainer intervention), not “external contributor.”
This avoids incorrect attribution and keeps the gate fail-closed without misleading audit comments.Suggested doc fix
-gh api "repos/<slug>/collaborators/<author>/permission" --jq '.permission' 2>/dev/null +perm_json=$(gh api -i "repos/<slug>/collaborators/<author>/permission" 2>/dev/null) || perm_rc=$? +# Parse HTTP status from headers, then: +# - 2xx: map .permission (admin|maintain|write => maintainer; read|none => external) +# - 404: external contributor +# - all other failures (403/429/5xx/auth/network): permission check failed -> do not auto-merge; add distinct maintainer-intervention comment#!/bin/bash # Verify whether the playbook currently distinguishes non-404 API failures. # Expected: explicit handling for 403/429/5xx/auth/network separate from external-contributor mapping. FILE=".agents/scripts/commands/pulse.md" echo "== Permission gate block ==" nl -ba "$FILE" | sed -n '64,90p' echo echo "== Look for explicit failure handling keywords ==" rg -n "403|429|5xx|permission check failed|manual|intervention|exit status|http status" "$FILE" || trueBased on learnings: Follow security protocols for all git and DevOps operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/commands/pulse.md around lines 67 - 71, The current gh api call ("gh api \"repos/<slug>/collaborators/<author>/permission\" --jq '.permission' 2>/dev/null") suppresses errors and therefore treats API failures as “external contributor”; change the logic to capture the command exit status and HTTP status and branch accordingly: if the command succeeds and output is admin|maintain|write treat as maintainer; if it succeeds and is read|none or HTTP 404 treat as external contributor and DO NOT auto-merge; if the command fails or returns HTTP 403/429/5xx or non-zero exit status treat as “permission check failed” and surface a manual-maintainer-intervention path (leave gate closed and post a request for human review), updating the textual guidance to list these three distinct outcomes. Use the existing gh api invocation as the lookup point and the permission values admin/maintain/write/read/none plus HTTP 404, 403, 429, and 5xx to implement the branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.agents/scripts/commands/pulse.md:
- Around line 67-71: The current gh api call ("gh api
\"repos/<slug>/collaborators/<author>/permission\" --jq '.permission'
2>/dev/null") suppresses errors and therefore treats API failures as “external
contributor”; change the logic to capture the command exit status and HTTP
status and branch accordingly: if the command succeeds and output is
admin|maintain|write treat as maintainer; if it succeeds and is read|none or
HTTP 404 treat as external contributor and DO NOT auto-merge; if the command
fails or returns HTTP 403/429/5xx or non-zero exit status treat as “permission
check failed” and surface a manual-maintainer-intervention path (leave gate
closed and post a request for human review), updating the textual guidance to
list these three distinct outcomes. Use the existing gh api invocation as the
lookup point and the permission values admin/maintain/write/read/none plus HTTP
404, 403, 429, and 5xx to implement the branches.
|
Pulse dispatch: Sending a worker to address CodeRabbit's CHANGES_REQUESTED items before merge:
Worker dispatched to fix both issues on branch |
|
Dispatching worker to address CodeRabbit's CHANGES_REQUESTED review:
Worker will push a fixup commit to |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 3 00:20:07 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Pulse review (2026-03-03T00:20:56Z): Both CodeRabbit issues from the initial review have been addressed in the latest commit (acfbf21):
CI still in-progress (Framework Validation, SonarCloud, Codacy, qlty). Will auto-merge once all checks pass and no blocking reviews remain. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.agents/scripts/commands/pulse.md (1)
67-70:⚠️ Potential issue | 🟠 MajorFail-closed branching is still not implementable from this snippet as written.
Line 67-70captures onlyperm+ process exit code while suppressing stderr. That does not let the flow reliably separate 404 external from 403/429/5xx/auth/network failure, so the three-outcome logic inLine 72-98can’t be enforced deterministically.Suggested fix (capture HTTP status explicitly)
-# Capture both output and exit code — do NOT discard stderr blindly -perm=$(gh api "repos/<slug>/collaborators/<author>/permission" --jq '.permission' 2>/dev/null) -rc=$? +# Capture HTTP status + body, then branch on status explicitly +resp=$(gh api -i "repos/<slug>/collaborators/<author>/permission" 2>&1) +rc=$? +status=$(printf '%s\n' "$resp" | sed -n '1s/.* \([0-9][0-9][0-9]\).*/\1/p') +body=$(printf '%s\n' "$resp" | sed -n '/^\r\?$/,$p' | tail -n +2) +perm=$(printf '%s\n' "$body" | jq -r '.permission // empty' 2>/dev/null) + +# Branching contract: +# status=200 + perm in admin|maintain|write -> maintainer +# status=200 + perm in read|none -> external +# status=404 -> external +# rc!=0 or other non-2xx/non-404 -> permission-check-failed (fail closed)GitHub CLI docs: how does `gh api` report HTTP 404 vs other failures (exit code behavior), and what is the recommended way to capture HTTP status + response body for branching logic?Based on learnings: Follow security protocols for all git and DevOps operations.
Also applies to: 72-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/commands/pulse.md around lines 67 - 70, The current snippet captures only perm and rc while discarding stderr, so you must capture the HTTP status and response body explicitly (don’t blindly redirect stderr to /dev/null) to allow deterministic three-way branching; change the gh api invocation that sets perm to instead collect both response body (assign to a response variable instead of perm directly) and the HTTP status code (assign to an http_code/http_status variable) — or call curl against the same endpoint to get body+status — then parse/extract the permission value into perm only when http_status==200, treat http_status==404 as the “not found” branch, and treat any 4xx/5xx or non-200/404 statuses (and non-zero exit code fallback) as the auth/network/error branch referenced by the logic handling perm and rc in the later decision block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.agents/scripts/commands/pulse.md:
- Around line 67-70: The current snippet captures only perm and rc while
discarding stderr, so you must capture the HTTP status and response body
explicitly (don’t blindly redirect stderr to /dev/null) to allow deterministic
three-way branching; change the gh api invocation that sets perm to instead
collect both response body (assign to a response variable instead of perm
directly) and the HTTP status code (assign to an http_code/http_status variable)
— or call curl against the same endpoint to get body+status — then parse/extract
the permission value into perm only when http_status==200, treat
http_status==404 as the “not found” branch, and treat any 4xx/5xx or non-200/404
statuses (and non-zero exit code fallback) as the auth/network/error branch
referenced by the logic handling perm and rc in the later decision block.
…ission check Replace 2>/dev/null pattern with gh api -i to capture HTTP headers, enabling explicit distinction between: - HTTP 200 + admin/maintain/write (maintainer, proceed) - HTTP 200 + read/none, or HTTP 404 (external contributor, label+skip) - HTTP 403/429/5xx or network failure (fail closed, manual intervention) The previous pattern suppressed stderr, making it impossible to distinguish 404 (external contributor) from 403/429/5xx (API failure). Addresses CodeRabbit CHANGES_REQUESTED on PR #2740.
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 3 00:26:00 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
2 participants
Summary
external-contributorlabel instead of auto-mergeWhy
PR #2737 was opened by an external contributor with content that shouldn't have been in the public repo. While it wasn't merged, the pulse had no gate to prevent auto-merging external PRs with green CI. This closes that gap.
How it works
Before any merge, the pulse checks:
admin,maintain,write→ maintainer, proceed normallyread,none, 404 → external contributor, comment + label + skipSummary by CodeRabbit