-
-
Notifications
You must be signed in to change notification settings - Fork 2
fix: improve handling of pull requests from forked repositories to prevent errors #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
512f5cf
8e83b62
b6497d5
fdae1ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| #!/usr/bin/env python3 | ||
| import json | ||
| import os | ||
| import sys | ||
| import subprocess | ||
| import re | ||
| from github import Github, Auth # type: ignore | ||
| from github import Github, Auth, GithubException # type: ignore | ||
|
|
||
|
|
||
| # Constants for message titles | ||
|
|
@@ -96,73 +97,102 @@ def add_job_summary() -> int: | |
| return 0 if result_text is None else 1 | ||
|
|
||
|
|
||
| def is_fork_pr() -> bool: | ||
| """Returns True when the triggering PR originates from a forked repository.""" | ||
| event_path = os.getenv("GITHUB_EVENT_PATH") | ||
| if not event_path: | ||
| return False | ||
| try: | ||
| with open(event_path, "r") as f: | ||
| event = json.load(f) | ||
| pr = event.get("pull_request", {}) | ||
| head_full_name = pr.get("head", {}).get("repo", {}).get("full_name", "") | ||
| base_full_name = pr.get("base", {}).get("repo", {}).get("full_name", "") | ||
| return bool( | ||
| head_full_name and base_full_name and head_full_name != base_full_name | ||
| ) | ||
| except Exception: | ||
| return False | ||
|
|
||
|
|
||
| def add_pr_comments() -> int: | ||
| """Posts the commit check result as a comment on the pull request.""" | ||
| if PR_COMMENTS == "false": | ||
| return 0 | ||
|
|
||
| # Fork PRs triggered by the pull_request event receive a read-only token; | ||
| # the GitHub API will always reject comment writes with 403. | ||
| if is_fork_pr(): | ||
shenxianpeng marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| print( | ||
| "::warning::Skipping PR comment: pull requests from forked repositories " | ||
| "cannot write comments via the pull_request event (GITHUB_TOKEN is " | ||
| "read-only for forks). Use the pull_request_target event or the " | ||
| "two-workflow artifact pattern instead. " | ||
| "See https://github.com/commit-check/commit-check-action/issues/77" | ||
| ) | ||
| return 0 | ||
|
Comment on lines
+123
to
+133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fork guard currently blocks valid Line 123 skips all fork PRs, including cases where the workflow runs on ✅ Proposed fix- if is_fork_pr():
+ event_name = os.getenv("GITHUB_EVENT_NAME", "")
+ if event_name == "pull_request" and is_fork_pr():
print(
"::warning::Skipping PR comment: pull requests from forked repositories "
"cannot write comments via the pull_request event (GITHUB_TOKEN is "
"read-only for forks). Use the pull_request_target event or the "
"two-workflow artifact pattern instead. "
"See https://github.com/commit-check/commit-check-action/issues/77"
)
return 0🤖 Prompt for AI Agents
shenxianpeng marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| try: | ||
| token = os.getenv("GITHUB_TOKEN") | ||
| repo_name = os.getenv("GITHUB_REPOSITORY") | ||
| pr_number = os.getenv("GITHUB_REF") | ||
| if pr_number is not None: | ||
| pr_number = pr_number.split("/")[-2] | ||
| else: | ||
| # Handle the case where GITHUB_REF is not set | ||
| raise ValueError("GITHUB_REF environment variable is not set") | ||
|
|
||
| # Initialize GitHub client | ||
| # Use new Auth API to avoid deprecation warning | ||
| if not token: | ||
| raise ValueError("GITHUB_TOKEN is not set") | ||
|
|
||
| g = Github(auth=Auth.Token(token)) | ||
| repo = g.get_repo(repo_name) | ||
| pull_request = repo.get_issue(int(pr_number)) | ||
|
|
||
| # Prepare comment content | ||
| result_text = read_result_file() | ||
| pr_comments = ( | ||
| pr_comment_body = ( | ||
| SUCCESS_TITLE | ||
| if result_text is None | ||
| else f"{FAILURE_TITLE}\n```\n{result_text}\n```" | ||
| ) | ||
|
|
||
| # Fetch all existing comments on the PR | ||
| comments = pull_request.get_comments() | ||
| matching_comments = [ | ||
| c | ||
| for c in comments | ||
| if c.body.startswith(SUCCESS_TITLE) or c.body.startswith(FAILURE_TITLE) | ||
| ] | ||
|
|
||
| # Track if we found a matching comment | ||
| matching_comments = [] | ||
| last_comment = None | ||
|
|
||
| for comment in comments: | ||
| if comment.body.startswith(SUCCESS_TITLE) or comment.body.startswith( | ||
| FAILURE_TITLE | ||
| ): | ||
| matching_comments.append(comment) | ||
| if matching_comments: | ||
| last_comment = matching_comments[-1] | ||
|
|
||
| if last_comment.body == pr_comments: | ||
| if last_comment.body == pr_comment_body: | ||
| print(f"PR comment already up-to-date for PR #{pr_number}.") | ||
| return 0 | ||
| else: | ||
| # If the last comment doesn't match, update it | ||
| print(f"Updating the last comment on PR #{pr_number}.") | ||
| last_comment.edit(pr_comments) | ||
|
|
||
| # Delete all older matching comments | ||
| print(f"Updating the last comment on PR #{pr_number}.") | ||
| last_comment.edit(pr_comment_body) | ||
| for comment in matching_comments[:-1]: | ||
| print(f"Deleting an old comment on PR #{pr_number}.") | ||
| comment.delete() | ||
|
Comment on lines
+161
to
176
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filter managed comments by author before edit/delete. Line 161-176 identifies “managed” comments only by body prefix, then edits/deletes them. This can mutate or delete human comments that happen to start with the same header. ✅ Suggested fix comments = pull_request.get_comments()
+ bot_login = repo.owner.login
matching_comments = [
c
for c in comments
- if c.body.startswith(SUCCESS_TITLE) or c.body.startswith(FAILURE_TITLE)
+ if (
+ c.user
+ and c.user.login == bot_login
+ and (
+ c.body.startswith(SUCCESS_TITLE)
+ or c.body.startswith(FAILURE_TITLE)
+ )
+ )
]🤖 Prompt for AI Agents |
||
| else: | ||
| # No matching comments, create a new one | ||
| print(f"Creating a new comment on PR #{pr_number}.") | ||
| pull_request.create_comment(body=pr_comments) | ||
| pull_request.create_comment(body=pr_comment_body) | ||
|
|
||
| return 0 if result_text is None else 1 | ||
| except GithubException as e: | ||
| if e.status == 403: | ||
| print( | ||
| "::warning::Unable to post PR comment (403 Forbidden). " | ||
| "Ensure your workflow grants 'issues: write' permission. " | ||
| f"Error: {e.data.get('message', str(e))}", | ||
| file=sys.stderr, | ||
| ) | ||
| return 0 | ||
| print(f"Error posting PR comment: {e}", file=sys.stderr) | ||
| return 0 | ||
| except Exception as e: | ||
| print(f"Error posting PR comment: {e}", file=sys.stderr) | ||
| return 1 | ||
| return 0 | ||
|
|
||
|
|
||
| def log_error_and_exit( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Narrow blind exception handlers to avoid masking real failures.
Line 112 and Line 191 both catch
Exceptionbroadly. This hides parse/runtime defects and makes behavior harder to diagnose in CI.💡 Proposed tightening
Also applies to: 191-193
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 112-112: Do not catch blind exception:
Exception(BLE001)
🤖 Prompt for AI Agents