fix: run E2E only when review is approved (secure for forks)#78
fix: run E2E only when review is approved (secure for forks)#78gh-simili-bot merged 5 commits intomainfrom
Conversation
Forked PRs fail because they lack access to secrets. Switching to
`pull_request_target` grants secret access but is risky.
To mitigate risk and respect user workflow:
1. Use `pull_request_target` (runs in context of base branch).
2. Explicitly checkout the PR code (`ref: head.sha`).
3. Require manual approval via `environment: e2e-test`.
- Maintainers must configure the 'e2e-test' environment in
Settings -> Environments -> Deployment protection rules
and add 'Required reviewers'.
- This ensures no untrusted code runs E2E without approval.
Fixes #77
Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
📝 WalkthroughWalkthroughChanged the E2E workflow triggers from Changes
Sequence Diagram(s)sequenceDiagram
participant Contributor as Contributor (PR)
participant Reviewer as Reviewer (submits review)
participant GitHub as GitHub Events
participant Actions as Actions Runner
participant Checkout as Checkout Step
participant TestRepo as External Test Repo (reporting / comments)
Contributor->>GitHub: Open PR (head SHA)
Reviewer->>GitHub: Submit review (approved)
GitHub->>Actions: Trigger workflow (pull_request_review) OR manual workflow_dispatch
Actions->>Checkout: Checkout repo using PR head SHA or `inputs.sha`
Checkout-->>Actions: PR code available (credential persistence disabled)
Actions->>TestRepo: Run tests, send PR_NUM/CO_SHA inputs for README/comment updates (use BOT_PAT when available)
TestRepo-->>Actions: Acknowledge comments/report updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Simili Triage ReportNote Quality Score: 9.5/10 (Excellent) Classification
Quality Improvements
Similar Threads
Warning Possible Duplicate (Confidence: 95%) Generated by Simili Bot |
Added `workflow_dispatch` to allow manual execution. This is useful for debugging trigger issues (like the pull_request -> pull_request_target transition) and for running tests on demand. Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/e2e-test.yml (2)
128-134:⚠️ Potential issue | 🟡 Minor
secrets.BOT_USERNAMEvsenv.BOT_NAMEinconsistency creates a latent correctness riskStep 4 constructs the repo path using
${{ secrets.BOT_USERNAME }}, while every other step (steps 1, 3, 5, 7–15) uses${{ env.BOT_NAME }}(hardcoded"gh-simili-bot"). IfBOT_USERNAMEsecret ever diverges fromBOT_NAME, step 4 will target a non-existent repo and silently fail or create issues in the wrong place.🐛 Proposed fix
- REPO="${{ secrets.BOT_USERNAME }}/${{ env.TEST_REPO_NAME }}" + REPO="${{ env.BOT_NAME }}/${{ env.TEST_REPO_NAME }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-test.yml around lines 128 - 134, The REPO variable in the "Create seed issues" step uses `${{ secrets.BOT_USERNAME }}` which is inconsistent with other steps that use `${{ env.BOT_NAME }}` and may point at the wrong repository; change the REPO assignment to use `${{ env.BOT_NAME }}` (i.e., set REPO to `${{ env.BOT_NAME }}/${{ env.TEST_REPO_NAME }}`) and verify there are no other occurrences of `secrets.BOT_USERNAME` in this workflow so all steps consistently reference `env.BOT_NAME` and `TEST_REPO_NAME`.
373-373:⚠️ Potential issue | 🟡 Minor
GITHUB_SHArefers to the base branch HEAD inpull_request_target, not the PR commitWith
pull_request_target,GITHUB_SHAis the HEAD of the base branch, not the PR's head SHA. The README written to the test repo will show a commit completely unrelated to the PR being tested. Usegithub.event.pull_request.head.sha(already referenced at line 160 and 255) for consistency.🐛 Proposed fix
- | **Commit** | \`${GITHUB_SHA:0:7}\` | + | **Commit** | \`${{ github.event.pull_request.head.sha }}\` |Note: since this is inside an unquoted heredoc,
${{ ... }}will be treated as a shell variable expansion and will fail. You need to either pass the SHA as a shell env var or escape it for the shell:BOT_OK="${{ steps.poll.outputs.bot_commented }}" + PR_SHA="${{ github.event.pull_request.head.sha }}" cat > README.md << RMEOF ... - | **Commit** | \`${GITHUB_SHA:0:7}\` | + | **Commit** | \`${PR_SHA:0:7}\` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-test.yml at line 373, The workflow uses GITHUB_SHA which in a pull_request_target context points to the base branch HEAD; replace that usage with the PR commit SHA via github.event.pull_request.head.sha where the commit is injected into the test README (replace `${GITHUB_SHA:0:7}` with the PR SHA). Because this value is being written inside an unquoted heredoc, either pass github.event.pull_request.head.sha into the shell as an env var (e.g., export PR_SHA=${{ github.event.pull_request.head.sha }}) and reference the shell var in the heredoc, or escape the GitHub expression so it isn’t expanded by the shell (e.g., use \${{ github.event.pull_request.head.sha }}), and ensure you still substring to 7 chars when writing the README.
🧹 Nitpick comments (1)
.github/workflows/e2e-test.yml (1)
346-354: Test repository created in step 1 is never deleted during cleanupThe Qdrant collection is cleaned up (
if: always()), but the test repository${{ env.BOT_NAME }}/${{ env.TEST_REPO_NAME }}created in step 1 is never removed. Every run leaves behind a permanent private repo under the bot account, which will accumulate over time and consume private repo quota.Consider adding a
gh repo deletestep in the cleanup phase:♻️ Proposed addition (after step 12)
# ----- 12b. Delete test repository ----- - name: Delete test repository if: always() env: GH_TOKEN: ${{ secrets.BOT_PAT }} run: | gh repo delete "${{ env.BOT_NAME }}/${{ env.TEST_REPO_NAME }}" --yes || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-test.yml around lines 346 - 354, Add a cleanup step to delete the test GitHub repository created in step 1 by invoking the GitHub CLI; specifically, add a new job step after the "Cleanup Qdrant collection" step that runs `gh repo delete "${{ env.BOT_NAME }}/${{ env.TEST_REPO_NAME }}" --yes || true`, set if: always(), and provide GH_TOKEN from secrets.BOT_PAT via env to authenticate (so the test repo named by env.BOT_NAME and env.TEST_REPO_NAME is removed on every run).
🤖 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/e2e-test.yml:
- Around line 19-21: The workflow uses pull_request_target with a later
actions/checkout that leaves credentials accessible; update the checkout step
(the actions/checkout invocation used when handling pull_request_target) to
include persist-credentials: false so the checked-out PR code cannot access
GITHUB_TOKEN, QDRANT_API_KEY, QDRANT_URL or GEMINI_API_KEY, and keep the
environment: e2e-test gate; also replace usages of GITHUB_SHA (the variable used
to log the tested commit) with the PR head commit reference
github.event.pull_request.head.sha to ensure the logged SHA corresponds to the
PR head rather than the base branch.
- Around line 40-41: The workflow sets environment: e2e-test for the job to
mitigate pull_request_target risks, but that only works if the GitHub
Environment "e2e-test" exists and has required reviewers configured; verify (and
if missing, create) the "e2e-test" environment in repository Settings →
Environments and enable Required reviewers so the job cannot run automatically
with secrets like BOT_PAT, QDRANT_API_KEY, GEMINI_API_KEY unless a reviewer
approves.
---
Outside diff comments:
In @.github/workflows/e2e-test.yml:
- Around line 128-134: The REPO variable in the "Create seed issues" step uses
`${{ secrets.BOT_USERNAME }}` which is inconsistent with other steps that use
`${{ env.BOT_NAME }}` and may point at the wrong repository; change the REPO
assignment to use `${{ env.BOT_NAME }}` (i.e., set REPO to `${{ env.BOT_NAME
}}/${{ env.TEST_REPO_NAME }}`) and verify there are no other occurrences of
`secrets.BOT_USERNAME` in this workflow so all steps consistently reference
`env.BOT_NAME` and `TEST_REPO_NAME`.
- Line 373: The workflow uses GITHUB_SHA which in a pull_request_target context
points to the base branch HEAD; replace that usage with the PR commit SHA via
github.event.pull_request.head.sha where the commit is injected into the test
README (replace `${GITHUB_SHA:0:7}` with the PR SHA). Because this value is
being written inside an unquoted heredoc, either pass
github.event.pull_request.head.sha into the shell as an env var (e.g., export
PR_SHA=${{ github.event.pull_request.head.sha }}) and reference the shell var in
the heredoc, or escape the GitHub expression so it isn’t expanded by the shell
(e.g., use \${{ github.event.pull_request.head.sha }}), and ensure you still
substring to 7 chars when writing the README.
---
Nitpick comments:
In @.github/workflows/e2e-test.yml:
- Around line 346-354: Add a cleanup step to delete the test GitHub repository
created in step 1 by invoking the GitHub CLI; specifically, add a new job step
after the "Cleanup Qdrant collection" step that runs `gh repo delete "${{
env.BOT_NAME }}/${{ env.TEST_REPO_NAME }}" --yes || true`, set if: always(), and
provide GH_TOKEN from secrets.BOT_PAT via env to authenticate (so the test repo
named by env.BOT_NAME and env.TEST_REPO_NAME is removed on every run).
.github/workflows/e2e-test.yml
Outdated
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the e2e-test environment exists and what protection rules it has
# First, let's check the workflow file itself to confirm context
cat .github/workflows/e2e-test.yml | head -50Repository: similigh/simili-bot
Length of output: 1825
🏁 Script executed:
#!/bin/bash
# Check repository environments and their protection rules
# Using simpler approach first - check if environment exists
gh api repos/similigh/simili-bot/environments \
--jq '.environments[] | select(.name == "e2e-test") | {name: .name, protection_rules: .protection_rules}'Repository: similigh/simili-bot
Length of output: 223
🏁 Script executed:
#!/bin/bash
# Search for references to e2e-test environment in the repo
echo "=== Searching for e2e-test references ==="
rg "e2e-test" --type yaml --type md --type txt
echo -e "\n=== Looking for environment documentation ==="
find . -type f \( -name "*environment*" -o -name "*ENVIRONMENT*" -o -name "*setup*" -o -name "*README*" \) ! -path "./.*" | head -20
echo -e "\n=== Checking for .github/workflows directory contents ==="
ls -la .github/workflows/ 2>/dev/null || echo "No workflows directory"Repository: similigh/simili-bot
Length of output: 689
🏁 Script executed:
#!/bin/bash
# Check the complete environment setup in the workflow
echo "=== Lines around environment: e2e-test ==="
sed -n '35,50p' .github/workflows/e2e-test.yml
echo -e "\n=== Search for all environment usages in workflows ==="
rg "environment:" .github/workflows/ -A 2
echo -e "\n=== Search for workflow_call or environment setup ==="
rg "(environment|environments|GITHUB_ENVIRONMENT)" . --type yaml --type md
echo -e "\n=== Check if there's a .github/environments directory ==="
ls -la .github/ 2>/dev/nullRepository: similigh/simili-bot
Length of output: 1942
🏁 Script executed:
#!/bin/bash
# Verify the security concern by checking the secrets accessed in the workflow
echo "=== Secrets used in e2e-test workflow ==="
rg "secrets\." .github/workflows/e2e-test.yml | head -10
echo -e "\n=== Check pull_request_target permissions ==="
sed -n '26,31p' .github/workflows/e2e-test.ymlRepository: similigh/simili-bot
Length of output: 990
Ensure the e2e-test environment exists in repository settings with required reviewers enabled.
environment: e2e-test at line 40 is the correct mitigation for pull_request_target, which allows untrusted PR code to run with access to sensitive secrets (BOT_PAT, QDRANT_API_KEY, GEMINI_API_KEY). However, this gate only works if properly configured:
- If the
e2e-testenvironment does not exist in repository settings, the job runs immediately without approval. - If the environment exists but has no required reviewers configured, it provides no protection.
The entire security guarantee depends on external repository configuration that cannot be verified from this workflow file. Before merging, confirm in Settings → Environments → e2e-test that required reviewers are enforced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/e2e-test.yml around lines 40 - 41, The workflow sets
environment: e2e-test for the job to mitigate pull_request_target risks, but
that only works if the GitHub Environment "e2e-test" exists and has required
reviewers configured; verify (and if missing, create) the "e2e-test" environment
in repository Settings → Environments and enable Required reviewers so the job
cannot run automatically with secrets like BOT_PAT, QDRANT_API_KEY,
GEMINI_API_KEY unless a reviewer approves.
Replaced `pull_request_target` trigger with `pull_request_review` (submitted). The workflow now runs only if: 1. A review is submitted. 2. The review state is 'approved'. This satisfies the requirement to run E2E tests (which access secrets) only after a maintainer has reviewed and approved the code changes. Manual `workflow_dispatch` trigger is retained for debugging. Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
Kavirubc
changed the title
Simili Triage ReportNote Quality Score: 9.5/10 (Excellent) Classification
Quality Improvements
Similar Threads
Warning Possible Duplicate (Confidence: 98%) Generated by Simili Bot |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e2e-test.yml:
- Around line 19-22: The workflow_dispatch trigger lacks inputs for PR SHA and
PR number causing github.event.pull_request.* to be empty; add inputs (e.g.,
pr_sha and pr_number) to the workflow_dispatch section and update all places
that currently reference github.event.pull_request.head.sha and
github.event.pull_request.number to use fallback expressions such as
github.event.pull_request.head.sha || github.event.inputs.pr_sha and
github.event.pull_request.number || github.event.inputs.pr_number; also update
the sed replacement that substitutes __PR_SHA__ and any gh pr comment invocation
to use the same fallback inputs so checkout (ref), action references, and gh
commands work when manually dispatched.
---
Duplicate comments:
In @.github/workflows/e2e-test.yml:
- Around line 40-42: The workflow's environment: e2e-test setting is ineffective
unless that GitHub Environment has Required reviewers configured; update the
repository settings to create the "e2e-test" environment and enable Required
reviewers (so PRs from forks cannot access BOT_PAT, QDRANT_API_KEY,
GEMINI_API_KEY), or alternatively remove/change the environment line in the
workflow (.github/workflows/e2e-test.yml) and/or move secret usage to a
protected path (e.g., run tests only from branches or use repository_dispatch
from trusted actors) to ensure forked PRs cannot access those secrets.
- Around line 157-162: Add persist-credentials: false to the actions/checkout@v4
step that checks out the PR head (the step using ref: ${{
github.event.pull_request.head.sha }} and path: simili-src) so no workflow token
is written into .git/config; also replace uses of the runtime env variable
GITHUB_SHA (logged elsewhere) with the explicit expression ${{
github.event.pull_request.head.sha }} to ensure the logged/tested commit is the
PR head rather than the base branch HEAD.
🧪 E2E Test❌ Bot responded: no Test repo → gh-simili-bot/simili-e2e-22146348741 Auto-generated by E2E pipeline |
1. **Security**: Added `persist-credentials: false` to checkout step to prevent GITHUB_TOKEN leakage (per CodeRabbit). 2. **Manual Trigger**: Fixed `workflow_dispatch` by adding inputs (`sha`, `pr_number`) and fallback logic. Previous version failed because `github.event.pull_request` is null. 3. **Traceability**: Updated logs, PR comments, and test repo README to show the actual checked-out commit SHA (`CO_SHA`) instead of the workflow SHA. Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
Switched from GITHUB_TOKEN to BOT_PAT for the PR commenting step. The GITHUB_TOKEN provided to workflows triggered by pull_request_review sometimes lacks write permissions on the PR, resulting in a 401 Bad Credentials error. BOT_PAT is confirmed to have the necessary scopes. Signed-off-by: Kavirubc <hapuarachchikaviru@gmail.com>
🧪 E2E Test✅ Bot responded: yes Test repo → gh-simili-bot/simili-e2e-22147187250 Auto-generated by E2E pipeline |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e-test.yml (1)
73-76:⚠️ Potential issue | 🟡 MinorMissing
inputs.pr_numberfallback in test repo description.When triggered via
workflow_dispatch,github.event.pull_request.numberis empty, producing the description"Simili E2E test – PR #". Consider using the same fallback pattern used elsewhere.gh repo create "${{ env.BOT_NAME }}/${{ env.TEST_REPO_NAME }}" \ --private \ - --description "Simili E2E test – PR #${{ github.event.pull_request.number }}" \ + --description "Simili E2E test – PR #${{ github.event.pull_request.number || inputs.pr_number }}" \ --clone🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-test.yml around lines 73 - 76, The repo creation description uses github.event.pull_request.number which is empty for workflow_dispatch; update the description string passed to the gh repo create command to fallback to inputs.pr_number when github.event.pull_request.number is unset (same pattern used elsewhere), so the description becomes something like "Simili E2E test – PR #${{ github.event.pull_request.number || github.event.inputs.pr_number }}" while keeping the existing env.BOT_NAME and env.TEST_REPO_NAME arguments.
🧹 Nitpick comments (1)
.github/workflows/e2e-test.yml (1)
162-163: Duplicate comment line.Line 162 and line 163 are identical step-section comments. Remove one.
- # ----- 5. Build CLI from PR & index seed issues ----- # ----- 5. Build CLI from PR & index seed issues -----🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-test.yml around lines 162 - 163, Remove the duplicated comment line "# ----- 5. Build CLI from PR & index seed issues -----" so only a single instance remains; find the two identical comment lines near the build/index step in the e2e-test workflow and delete one of them to avoid the duplicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/e2e-test.yml:
- Around line 73-76: The repo creation description uses
github.event.pull_request.number which is empty for workflow_dispatch; update
the description string passed to the gh repo create command to fallback to
inputs.pr_number when github.event.pull_request.number is unset (same pattern
used elsewhere), so the description becomes something like "Simili E2E test – PR
#${{ github.event.pull_request.number || github.event.inputs.pr_number }}" while
keeping the existing env.BOT_NAME and env.TEST_REPO_NAME arguments.
---
Nitpick comments:
In @.github/workflows/e2e-test.yml:
- Around line 162-163: Remove the duplicated comment line "# ----- 5. Build CLI
from PR & index seed issues -----" so only a single instance remains; find the
two identical comment lines near the build/index step in the e2e-test workflow
and delete one of them to avoid the duplicate.
2 participants
Problem
Forked PRs trigger
pull_request, which has no access to secrets. This causes E2E failure. Usingpull_request_targetgrants secret access but is risky if run automatically.Solution
Switch trigger to
pull_request_review(submitted).ref: head.sha) to test the changes.How to Test
workflow_dispatch) for debugging.Fixes #77
Summary by CodeRabbit