fix: handle fork PR permission issue for pr-comments gracefully#215
Conversation
📝 WalkthroughWalkthrough
ChangesFork PR Comment Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
Commit-Check ❌ |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.py`:
- Around line 273-294: The is_fork_pr() check in the add_pr_comments() function
unconditionally blocks all fork PRs from posting comments, but this should only
apply to the pull_request event which has read-only token permissions. For
pull_request_target events, the token has write permissions and should be able
to post comments on fork PRs. Modify the condition on line 273 to check both
is_fork_pr() AND that the current event type is pull_request (not
pull_request_target), so the skip only applies to the pull_request event and
allows pull_request_target to proceed with posting comments on forks.
In `@README.md`:
- Around line 184-196: The fenced code block containing the ASCII diagram in
README.md (lines 184-196) is missing a language tag, which violates markdownlint
rule MD040. Add a language identifier such as `text` to the opening fence
(change ``` to ```text) to specify the code block format and resolve the lint
warning.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ff01b3e-8314-404e-92e6-457fe0b5d7e6
📒 Files selected for processing (3)
README.mdmain.pymain_test.py
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 (1)
main_test.py (1)
488-499:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest relies on implicit
GITHUB_EVENT_NAMEstate.This test patches
main.is_fork_prbut doesn't setGITHUB_EVENT_NAME. Sinceis_fork_pr_with_readonly_token()checks both conditions (is_fork_pr() and event != "pull_request_target"), the test's behavior depends on the ambient environment. IfGITHUB_EVENT_NAMEhappens to be"pull_request_target"in CI or locally, the skip branch won't execute and the test may fail.🛠️ Suggested fix
def test_fork_pr_skips_comment_and_warns(self): with ( patch("main.PR_COMMENTS_ENABLED", True), patch("main.is_fork_pr", return_value=True), patch("main.JOB_SUMMARY_ENABLED", False), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), patch("builtins.print") as mock_print, ): rc = main.add_pr_comments()Apply the same fix to
test_fork_pr_writes_job_summary_hinton line 503.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main_test.py` around lines 488 - 499, The test_fork_pr_skips_comment_and_warns function patches is_fork_pr but does not explicitly set the GITHUB_EVENT_NAME environment variable. Since is_fork_pr_with_readonly_token() checks both the is_fork_pr() condition and whether the event is not "pull_request_target", the test behavior depends on whatever GITHUB_EVENT_NAME happens to be in the ambient environment, making the test non-deterministic. Add a patch for the GITHUB_EVENT_NAME environment variable (set it to "pull_request" to represent a normal fork PR scenario) to ensure the test always executes the expected code path regardless of the CI or local environment. Apply the same fix to test_fork_pr_writes_job_summary_hint at line 503.
🧹 Nitpick comments (1)
main.py (1)
298-299: 💤 Low valueConsider logging the exception before falling through.
The silent
except Exception: passdiscards diagnostic context (e.g., JSON decode errors, permission issues). This can make debugging harder when theValueErroris raised without indicating what went wrong in the fallback path.💡 Suggested improvement
- except Exception: - pass + except Exception as e: + # Log but continue to raise ValueError with clearer message + print(f"::debug::Failed to read PR number from event payload: {e}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 298 - 299, The bare except Exception: pass statement at lines 298-299 silently discards exception information without any logging, which makes debugging difficult when issues occur. Replace this silent pass statement by adding a log statement that captures and records the exception details (using the exception object) before allowing the code to continue. This will preserve diagnostic context such as error types and messages for troubleshooting while still maintaining the fallback behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/commit-check-workflow-a.yml`:
- Around line 30-33: The `actions/upload-artifact` step only runs on success,
but when the `commit-check-action` exits with a non-zero code on failure, the
artifact upload gets skipped, preventing Workflow B from downloading the
result.txt file. Add an `if: always()` condition to the
`actions/upload-artifact@v4` step to ensure it runs regardless of whether
previous steps succeeded or failed, allowing the artifact to be uploaded in all
cases.
In `@examples/commit-check-workflow-b.yml`:
- Around line 23-25: The workflow file's permissions block is missing the
required `issues: write` permission for PR comment operations. The workflow
currently uses `github.rest.issues.listComments()`,
`github.rest.issues.updateComment()`, and `github.rest.issues.createComment()`
which all require the `issues: write` permission to function without 403
Forbidden errors. Add `issues: write` to the permissions section alongside the
existing `pull-requests: write` and `actions: read` permissions in the
example/commit-check-workflow-b.yml file.
---
Outside diff comments:
In `@main_test.py`:
- Around line 488-499: The test_fork_pr_skips_comment_and_warns function patches
is_fork_pr but does not explicitly set the GITHUB_EVENT_NAME environment
variable. Since is_fork_pr_with_readonly_token() checks both the is_fork_pr()
condition and whether the event is not "pull_request_target", the test behavior
depends on whatever GITHUB_EVENT_NAME happens to be in the ambient environment,
making the test non-deterministic. Add a patch for the GITHUB_EVENT_NAME
environment variable (set it to "pull_request" to represent a normal fork PR
scenario) to ensure the test always executes the expected code path regardless
of the CI or local environment. Apply the same fix to
test_fork_pr_writes_job_summary_hint at line 503.
---
Nitpick comments:
In `@main.py`:
- Around line 298-299: The bare except Exception: pass statement at lines
298-299 silently discards exception information without any logging, which makes
debugging difficult when issues occur. Replace this silent pass statement by
adding a log statement that captures and records the exception details (using
the exception object) before allowing the code to continue. This will preserve
diagnostic context such as error types and messages for troubleshooting while
still maintaining the fallback behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f44167b-0f43-415d-a52e-a10d2a2a5107
📒 Files selected for processing (5)
README.mdexamples/commit-check-workflow-a.ymlexamples/commit-check-workflow-b.ymlmain.pymain_test.py
| - uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: commit-check-result-${{ github.event.number }} | ||
| path: result.txt |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the workflow file
find . -name "*commit-check-workflow-a.yml" -o -name "*workflow*.yml" | head -20Repository: commit-check/commit-check-action
Length of output: 154
🏁 Script executed:
# Let's search for any workflow files in the examples directory
fd -e yml -e yaml . examples/ 2>/dev/null || echo "examples dir search"Repository: commit-check/commit-check-action
Length of output: 150
🌐 Web query:
GitHub Actions default step behavior on failure - do steps skip by default when previous steps fail
💡 Result:
Yes, GitHub Actions steps are skipped by default when a previous step fails [1][2]. By default, every step in a job has an implicit condition of success, which means subsequent steps will only execute if all previous steps have completed successfully [2]. If a step fails (i.e., returns a non-zero exit code), the runner stops executing the remaining steps in that job [1][3]. You can override this default behavior using the following methods: 1. always conditional: You can add if: always to a step to ensure it executes regardless of whether previous steps succeeded or failed [4][2]. This is commonly used for cleanup or reporting tasks [4][2]. 2. continue-on-error: If you set continue-on-error: true on a specific step, the job will treat that step as a success even if it fails, allowing subsequent steps to execute normally [1][3]. Note that the job's final status will still reflect the failure unless otherwise handled [5]. 3. Conditional logic with outcome: You can use the steps.<step_id>.outcome context to check if a specific previous step failed and run subsequent steps conditionally, such as using if: failure && steps..outcome == 'failure' [5][2][3]. Additionally, note that shell steps (bash/sh) in GitHub Actions run with set -e enabled by default, which ensures that a non-zero exit code in any part of a command script triggers the step failure [6].
Citations:
- 1: https://notes.kodekloud.com/docs/GitHub-Actions-Certification/Continuous-Integration-with-GitHub-Actions/Using-continue-on-error-expression/page
- 2: https://notes.kodekloud.com/docs/GitHub-Actions/Continuous-Integration-with-GitHub-Actions/Using-if-expressions-with-Step-contexts/page
- 3: https://dev.to/thisdotmedia/how-to-retry-failed-steps-in-github-action-workflows-fkl
- 4: https://stackoverflow.com/questions/58858429/how-to-run-a-github-actions-step-even-if-the-previous-step-fails-while-still-f
- 5: Wrong behaviour when combining 'continue-on-error' and 'failure()' in subsequent steps actions/toolkit#1034
- 6: https://stackoverflow.com/questions/75419587/does-a-github-action-step-use-set-e-semantics-by-default
🌐 Web query:
GitHub Actions if: always() condition when to use conditional steps
💡 Result:
In GitHub Actions, the always function is a conditional status check that causes a step or job to execute regardless of the status of preceding steps or jobs, including when the workflow is canceled [1][2][3]. When to use always Use always for tasks that must run regardless of the outcome of previous steps, such as: - Sending notifications or logs [1][2]. - Uploading test artifacts or diagnostics after a failure [4]. - Performing essential cleanup operations [5]. Important Considerations and Alternatives While always is powerful, it should be used with caution because it overrides the default behavior where subsequent steps are skipped if a previous step fails or is canceled [1][2][6]. 1. Cancellation Behavior: Because always returns true even if the workflow is canceled, it can trigger tasks you might not want to run during a cancellation, such as deployment or external notifications [4][6]. 2. Critical Failures: It is recommended to avoid using always for tasks that could suffer from critical failures (e.g., source checkout), as the workflow might hang until it reaches a timeout [1][2]. 3. Recommended Alternative: If you want a step to run regardless of success or failure but still respect a user's decision to cancel the workflow, use the following conditional instead [1][6]: if: ${{ success || failure }} This alternative ensures the step runs if the previous steps succeeded or failed, but prevents it from running if the job itself was explicitly canceled [4][6]. Technical Context By default, GitHub Actions applies a hidden success check to all steps [2]. If you provide an if: condition, you must explicitly include status functions if you want to override this default behavior. For example, if you want a step to run only on failure, you must use if: ${{ failure }} [1][7]. Always effectively overrides all default status checks, ensuring execution [1][2].
Citations:
- 1: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/expressions.md
- 2: https://docs.github.com/en/enterprise-server@3.4/actions/learn-github-actions/expressions
- 3: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions
- 4: https://ghlint.twisterrob.net/issues/default/NeverUseAlways/
- 5: https://oneuptime.com/blog/post/2025-12-20-conditional-steps-github-actions/view
- 6: https://www.adnathanail.dev/posts/_2023-01-07-github-actions-always.mdx
- 7: https://timesofcloud.com/github-actions/conditionals/
🏁 Script executed:
# Read the full commit-check-workflow-a.yml file
cat -n examples/commit-check-workflow-a.ymlRepository: commit-check/commit-check-action
Length of output: 1252
🏁 Script executed:
# Read the full commit-check-workflow-b.yml file to understand the dependency
cat -n examples/commit-check-workflow-b.ymlRepository: commit-check/commit-check-action
Length of output: 2864
🏁 Script executed:
# Search for information about the commit-check-action behavior
# Check if there's any documentation or code about the action
find . -name "action.yml" -o -name "action.yaml" | head -5Repository: commit-check/commit-check-action
Length of output: 89
🏁 Script executed:
# Look for README or documentation that explains the action behavior
fd -e md . | xargs grep -l "commit-check-action" | head -5Repository: commit-check/commit-check-action
Length of output: 86
🌐 Web query:
commit-check-action exit code behavior on failed checks - does it fail the step or just output results
💡 Result:
The commit-check-action is designed to fail the GitHub Actions step when checks fail [1][2]. By default, when commit-check-action identifies a failed validation, it exits with a non-zero exit code (typically 1), which causes the GitHub Actions step to be marked as failed [1][3][2]. In addition to failing the step, the action also outputs detailed information about the failed checks, which can be viewed in the workflow logs or on the job summary page [2][4]. If you wish to prevent the step from failing while still running the checks (for example, to see results without stopping the workflow), you can use the dry-run mode [1][5]. When dry-run is enabled, the action will output the results of the checks but will always return an exit code of 0, allowing the step to pass regardless of the validation outcome [1][5][2].
Citations:
- 1: https://github.com/commit-check/commit-check-action
- 2: https://github.com/marketplace/actions/commit-check-action
- 3: https://commit-check.github.io/commit-check/
- 4: https://commit-check.github.io/commit-check/index.html
- 5: https://commit-check.github.io/commit-check/cli_args.html
Ensure artifact upload runs even when checks fail.
actions/upload-artifact currently runs only on success. When commit-check-action detects failures (at line 22), it exits with a non-zero code, causing the subsequent upload step to be skipped. This breaks Workflow B's ability to post a PR comment on failing checks, since it cannot download the missing result.txt artifact.
Proposed fix
# Save results so Workflow B can post a PR comment
- uses: actions/upload-artifact@v4
+ if: always()
with:
name: commit-check-result-${{ github.event.number }}
path: result.txt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/commit-check-workflow-a.yml` around lines 30 - 33, The
`actions/upload-artifact` step only runs on success, but when the
`commit-check-action` exits with a non-zero code on failure, the artifact upload
gets skipped, preventing Workflow B from downloading the result.txt file. Add an
`if: always()` condition to the `actions/upload-artifact@v4` step to ensure it
runs regardless of whether previous steps succeeded or failed, allowing the
artifact to be uploaded in all cases.
| permissions: | ||
| pull-requests: write | ||
| actions: read # needed to download artifacts |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "commit-check-workflow-b.yml" -type fRepository: commit-check/commit-check-action
Length of output: 115
🏁 Script executed:
cat -n ./examples/commit-check-workflow-b.ymlRepository: commit-check/commit-check-action
Length of output: 2864
🌐 Web query:
GitHub API permissions createComment updateComment listComments issues:write pull-requests:write
💡 Result:
To interact with issue and pull request comments via the GitHub REST API, the required permissions primarily fall under the Issues and Pull Requests categories [1][2]. For standard issue comments and pull request timeline comments, the required permission is generally 'issues' [2]. Creating, updating, and listing these comments typically requires 'write' access to the 'issues' permission [2]. Because every pull request is considered an issue by the API, the 'issues' permission covers comment actions across both [1]. For more specific comment types, requirements may differ: 1. Issue Comments (including PR timeline comments): Requires 'issues' permission ('read' for listing, 'write' for creating/updating) [1][2]. 2. Pull Request Review Comments (comments on the diff): Requires 'pull_requests' permission ('read' for listing, 'write' for creating/updating) [2][3]. 3. Commit Comments: Often requires 'contents' permission ('read' for listing, 'write' for creating/updating) [4][5]. When configuring fine-grained personal access tokens or GitHub App permissions: - 'Write' access always includes 'read' access [2]. - If you are unsure of the specific requirements for a particular endpoint, you can check the 'X-Accepted-GitHub-Permissions' header returned in the API response, which explicitly states the necessary permissions for that call [6][7][8]. - For GitHub Actions workflows using the 'GITHUB_TOKEN', you can define these permissions in your workflow YAML file using the 'permissions' key (e.g., 'issues: write' and 'pull-requests: write') [9].
Citations:
- 1: https://docs.github.com/en/rest/issues/comments?apiVersion=--
- 2: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens?WT.mc_id=25daysofserverless-github-jopapa
- 3: https://docs.github.com/rest/pulls/comments
- 4: https://developer.github.com/v3/repos/comments
- 5: https://docs.github.com/enterprise-server@3.13/rest/commits/comments
- 6: https://docs.github.com/en/rest/authentication/permissions-required-for-fine-grained-personal-access-tokens?apiVersion=2022-11-28
- 7: https://docs.github.com/en/enterprise-cloud@latest/rest/authentication/permissions-required-for-github-apps
- 8: https://docs.github.com/en/rest/using-the-rest-api/troubleshooting-the-rest-api?algolia-query=third%2Bparty&apiVersion=2022-11-28
- 9: https://github.com/github/docs/blob/main/content/rest/authentication/authenticating-to-the-rest-api.md
Add issues: write permission for PR comment operations.
This workflow uses github.rest.issues.listComments(), github.rest.issues.updateComment(), and github.rest.issues.createComment() to post and update PR comments. These operations require the issues: write permission; pull-requests: write alone will result in 403 Forbidden errors.
Proposed fix
permissions:
+ issues: write
pull-requests: write
actions: read # needed to download artifacts📝 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: | |
| pull-requests: write | |
| actions: read # needed to download artifacts | |
| permissions: | |
| issues: write | |
| pull-requests: write | |
| actions: read # needed to download artifacts |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/commit-check-workflow-b.yml` around lines 23 - 25, The workflow
file's permissions block is missing the required `issues: write` permission for
PR comment operations. The workflow currently uses
`github.rest.issues.listComments()`, `github.rest.issues.updateComment()`, and
`github.rest.issues.createComment()` which all require the `issues: write`
permission to function without 403 Forbidden errors. Add `issues: write` to the
permissions section alongside the existing `pull-requests: write` and `actions:
read` permissions in the example/commit-check-workflow-b.yml file.
shenxianpeng
force-pushed
the
feature/fork-pr-comments-workflow-run
branch
from
11df90c to
1b793ee
Compare
…n pattern - Add Job Summary notice when PR comments are skipped for fork PRs (not just a ::warning:: log line, but also visible in GH UI) - Update warning message to reference new documentation section - Add comprehensive Fork PR Comments section to README with: - Two-workflow (workflow_run) pattern as recommended best practice - pull_request_target alternative with security warnings - Add unit tests for the new Job Summary behavior Closes #143
Code now links to docs/fork-pr-comments.md (a fixed file path) instead of README section anchors (#fork-pr-comments). This prevents link rot when the README gets restructured. README keeps a brief summary with a link to the dedicated docs file.
shenxianpeng
force-pushed
the
feature/fork-pr-comments-workflow-run
branch
from
1b793ee to
b6390fc
Compare
shenxianpeng
changed the title
shenxianpeng
added
bug
1 participant
Problem
When
pr-comments: trueis set, fork PRs crash with a 403 error becauseGITHUB_TOKENis read-only onpull_requestevents from forks (GitHub security policy).Changes
Graceful degradation — Instead of a 403 crash, fork PRs now get:
::warning::in the logs explaining why the comment was skippedBug fixes:
pull_request_targetnow works with fork PRs (was incorrectly blocked byis_fork_pr())pull_request_target(falls back to event payload whenGITHUB_REFis not parseable)Documentation — New
docs/fork-pr-comments.mdwith two approaches:workflow_run(recommended, zero security risk)pull_request_target(advanced, use with caution)Ready-to-use workflow files in
examples/.Related Issues
Closes #143 — replaces the 403 crash with graceful handling and provides clear migration paths.
Closes #77