require approval only for fork PRs #2004
Conversation
haritamar
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
WalkthroughAdds a new job Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GitHub as "GitHub Event"
participant Checker as "check-if-requires-approval (job)"
participant Test as "test (job)"
GitHub->>Checker: Trigger (pull_request_target / workflow event)
Checker->>Checker: Compare head repo vs base repo
Note right of Checker #DDEBF7: sets outputs.requires_approval = "true" if head repo != base repo
Checker-->>GitHub: outputs.requires_approval
GitHub->>Test: Start (needs: check-if-requires-approval)
Test->>Test: Read needs.check-if-requires-approval.outputs.requires_approval
alt requires_approval == 'true'
Note right of Test #E8F6E8: environment = elementary_test_env
else requires_approval != 'true'
Note right of Test #F9F0F0: no environment set
end
Test-->GitHub: Run remaining test steps (unchanged)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
Comment |
|
👋 @haritamar |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/test-warehouse.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code-quality
.github/workflows/test-warehouse.yml
Outdated
| detect-fork: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| requires_approval: ${{ steps.set-output.outputs.requires_approval }} | ||
| steps: | ||
| - name: Set fork output | ||
| id: set-output | ||
| run: | | ||
| if [[ "${{ github.event_name }}" == "pull_request_target" && "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then | ||
| echo "requires_approval=true" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "requires_approval=false" >> $GITHUB_OUTPUT | ||
| fi | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fork detection will always be false when invoked via workflow_call (security risk).
In reusable workflows, github.event_name is workflow_call, so your pull_request_target check never matches and requires_approval stays false even for fork PRs. That bypasses the protected environment and exposes secrets to forked code. Fix by accepting a boolean input from the caller (e.g., is-fork) and/or gating on the presence of a PR payload.
Apply one of these diffs (preferred: combine both for robustness):
Option A — rely on caller-provided flag:
@@
workflow_call:
inputs:
@@
generate-data:
type: boolean
required: false
default: false
+ is-fork:
+ type: boolean
+ required: false
+ default: false
@@
detect-fork:
@@
- name: Set fork output
id: set-output
run: |
- if [[ "${{ github.event_name }}" == "pull_request_target" && "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then
- echo "requires_approval=true" >> $GITHUB_OUTPUT
- else
- echo "requires_approval=false" >> $GITHUB_OUTPUT
- fi
+ if [[ "${{ inputs.is-fork }}" == "true" ]]; then
+ echo "requires_approval=true" >> "$GITHUB_OUTPUT"
+ else
+ echo "requires_approval=false" >> "$GITHUB_OUTPUT"
+ fiOption B — autodetect when PR payload exists (works when this workflow is triggered directly by PR events):
@@
- name: Set fork output
id: set-output
run: |
- if [[ "${{ github.event_name }}" == "pull_request_target" && "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then
+ if [[ -n "${{ github.event.pull_request.head.repo.full_name }}" && "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then
echo "requires_approval=true" >> $GITHUB_OUTPUT
else
echo "requires_approval=false" >> $GITHUB_OUTPUT
fiOption C — combined (use input when provided, else fallback to autodetect):
@@
- name: Set fork output
id: set-output
run: |
- if [[ "${{ github.event_name }}" == "pull_request_target" && "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then
+ if [[ "${{ inputs.is-fork }}" == "true" ]] || [[ -n "${{ github.event.pull_request.head.repo.full_name }}" && "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then
echo "requires_approval=true" >> $GITHUB_OUTPUT
else
echo "requires_approval=false" >> $GITHUB_OUTPUT
fiIf you choose Option A/C, ensure the caller sets:
with:
is-fork: ${{ github.event.pull_request.head.repo.full_name != github.repository }}
📝 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.
| detect-fork: | |
| runs-on: ubuntu-latest | |
| outputs: | |
| requires_approval: ${{ steps.set-output.outputs.requires_approval }} | |
| steps: | |
| - name: Set fork output | |
| id: set-output | |
| run: | | |
| if [[ "${{ github.event_name }}" == "pull_request_target" && "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then | |
| echo "requires_approval=true" >> $GITHUB_OUTPUT | |
| else | |
| echo "requires_approval=false" >> $GITHUB_OUTPUT | |
| fi | |
| detect-fork: | |
| runs-on: ubuntu-latest | |
| outputs: | |
| requires_approval: ${{ steps.set-output.outputs.requires_approval }} | |
| steps: | |
| - name: Set fork output | |
| id: set-output | |
| run: | | |
| if [[ "${{ inputs.is-fork }}" == "true" ]] || [[ -n "${{ github.event.pull_request.head.repo.full_name }}" && "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]]; then | |
| echo "requires_approval=true" >> $GITHUB_OUTPUT | |
| else | |
| echo "requires_approval=false" >> $GITHUB_OUTPUT | |
| fi |
🤖 Prompt for AI Agents
In .github/workflows/test-warehouse.yml around lines 78–91, the current
fork-detection logic relies solely on github.event_name == "pull_request_target"
which always fails for reusable workflows (workflow_call) and leaves
requires_approval=false; change the step to accept a boolean input (e.g.,
is-fork) from the caller and/or fallback to autodetection: add an optional
workflow input named is-fork, set requires_approval=true when is-fork == "true";
if is-fork is not provided, detect a PR payload by checking for
github.event.pull_request and compare
github.event.pull_request.head.repo.full_name != github.repository to decide
requires_approval; ensure callers set is-fork when invoking workflow_call (with:
is-fork: ${{ github.event.pull_request.head.repo.full_name != github.repository
}}).
.github/workflows/test-warehouse.yml
Outdated
| needs: [detect-fork] | ||
| # PRs from forks require approval - so we use an environment that requires an approver from the team. | ||
| environment: ${{ (needs.detect-fork.outputs.requires_approval == 'true' && 'elementary_test_env') || '' }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
environment: '' is invalid; job may fail or create an empty-named environment.
GitHub requires a non-empty environment name. Using '' risks a config error. Provide a safe fallback (an unprotected environment) or split into two jobs.
Minimal change (use an unprotected “ci” environment as the default):
- environment: ${{ (needs.detect-fork.outputs.requires_approval == 'true' && 'elementary_test_env') || '' }}
+ environment: ${{ (needs.detect-fork.outputs.requires_approval == 'true' && 'elementary_test_env') || 'ci' }}Make sure the “ci” environment exists (no required reviewers), or let GitHub auto-create it without protections.
Alternative (no fallback env, zero risk of accidental approvals): duplicate the job as test-protected (with environment: elementary_test_env and if: needs.detect-fork.outputs.requires_approval == 'true') and test-unprotected (no environment and if: ... != 'true'). I can draft that if you prefer.
📝 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.
| needs: [detect-fork] | |
| # PRs from forks require approval - so we use an environment that requires an approver from the team. | |
| environment: ${{ (needs.detect-fork.outputs.requires_approval == 'true' && 'elementary_test_env') || '' }} | |
| needs: [detect-fork] | |
| # PRs from forks require approval - so we use an environment that requires an approver from the team. | |
| environment: ${{ (needs.detect-fork.outputs.requires_approval == 'true' && 'elementary_test_env') || 'ci' }} |
🤖 Prompt for AI Agents
.github/workflows/test-warehouse.yml lines 94-96: the conditional currently can
yield an empty string for environment which GitHub rejects; change the fallback
to a valid unprotected environment name (e.g., replace the empty string with
'ci' so the line reads use 'ci' as the default), and ensure a non-protected "ci"
environment exists (or is acceptable to be auto-created); alternatively, if you
prefer no fallback, split the job into two: one job for protected runs with
environment: elementary_test_env gated by
needs.detect-fork.outputs.requires_approval == 'true' and a second job without
any environment gated by the inverse condition.
haritamar
deleted the
ele-5051-only-require-approval-on-prs-from-forks
branch
null
Summary by CodeRabbit