fix: improve handling of pull requests from forked repositories to prevent errors#182
Merged
shenxianpeng merged 4 commits intomainfrom Mar 3, 2026
Merged
Conversation
Contributor
Commit-Check ✔️ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Changes
Sequence DiagramsequenceDiagram
participant GA as GitHub Actions
participant WF as Workflow (main.py)
participant GH as GitHub API
GA->>WF: PR event triggers script
WF->>WF: is_fork_pr() reads GITHUB_EVENT_PATH (JSON)
alt Fork PR detected
WF->>WF: Log warning "skipping PR comments for fork"
WF-->>GA: Exit gracefully (no comment attempts)
else Non-fork PR
WF->>WF: Build new comment body
WF->>WF: Collect matching_comments (list comprehension)
alt matching_comments exist
WF->>WF: Compare latest match with new body
alt Different
WF->>GH: Update latest comment
WF->>GH: Delete older matching comments
GH-->>WF: Reply (or raise GithubException)
else Same
WF-->>GA: No API calls needed
end
else No matches
WF->>GH: Create new comment
GH-->>WF: Reply (or raise GithubException)
end
alt GithubException (403)
WF->>WF: Log user-facing warning and return
WF-->>GA: Continue without failing
else Other GithubException
WF->>WF: Log and return
WF-->>GA: Continue
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 |
shenxianpeng
changed the title
![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 (1)
main.py (1)
1-230:⚠️ Potential issue | 🟡 MinorCI is failing due to Black formatting in this file.
Run pre-commitreportsmain.pyneeds formatting. Please runpre-commit run --all-files(orblack main.py) and commit the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 1 - 230, Black/pre-commit flagged formatting issues in main.py; run the formatter and commit the changes. Fix by running "pre-commit run --all-files" or "black main.py" locally, stage and commit the updated file, ensuring formatting changes are applied across imports, function signatures (e.g., run_commit_check, read_result_file, add_job_summary, add_pr_comments, log_error_and_exit), and long string concatenations so the CI pre-commit check passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 112-113: The broad "except Exception:" handlers (the exact "except
Exception:" at the shown location and the similar block at lines 191-193) should
be narrowed to only the exceptions you expect from the guarded code (for example
json.JSONDecodeError / ValueError for parsing, KeyError for dict access,
TypeError for bad types, or specific library errors) and capture the exception
as a variable (e.g., "except (ValueError, json.JSONDecodeError) as e:") so you
can log e before returning False; for truly unexpected errors re-raise them
instead of swallowing (or let them propagate) so CI can surface real failures.
Ensure you replace each "except Exception:" with the specific exception tuple
relevant to the surrounding parse/validation logic and add logging of the
exception object.
- Around line 121-131: The current fork guard unconditionally returns for any
fork PR (is_fork_pr()), which also blocks valid pull_request_target workflows;
change the conditional so it only skips when the event is a fork PR AND the
workflow is NOT running under pull_request_target (e.g., check
os.getenv("GITHUB_EVENT_NAME") != "pull_request_target" or equivalent), leaving
the warning and return in the is_fork_pr() block but permitting comment writes
when the event is pull_request_target (optionally also guard by an explicit
writable token/permission check if available).
---
Outside diff comments:
In `@main.py`:
- Around line 1-230: Black/pre-commit flagged formatting issues in main.py; run
the formatter and commit the changes. Fix by running "pre-commit run
--all-files" or "black main.py" locally, stage and commit the updated file,
ensuring formatting changes are applied across imports, function signatures
(e.g., run_commit_check, read_result_file, add_job_summary, add_pr_comments,
log_error_and_exit), and long string concatenations so the CI pre-commit check
passes.
Comment on lines
+112
to
+113
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Narrow blind exception handlers to avoid masking real failures.
Line 112 and Line 191 both catch Exception broadly. This hides parse/runtime defects and makes behavior harder to diagnose in CI.
💡 Proposed tightening
@@
- except Exception:
- return False
+ except (OSError, json.JSONDecodeError) as e:
+ print(f"::warning::Unable to parse GITHUB_EVENT_PATH: {e}", file=sys.stderr)
+ return False
@@
- except Exception as e:
- print(f"Error posting PR comment: {e}", file=sys.stderr)
- return 0
+ except Exception as e:
+ print(f"Unexpected error posting PR comment: {e}", file=sys.stderr)
+ raiseAlso applies to: 191-193
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 112-112: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main.py` around lines 112 - 113, The broad "except Exception:" handlers (the
exact "except Exception:" at the shown location and the similar block at lines
191-193) should be narrowed to only the exceptions you expect from the guarded
code (for example json.JSONDecodeError / ValueError for parsing, KeyError for
dict access, TypeError for bad types, or specific library errors) and capture
the exception as a variable (e.g., "except (ValueError, json.JSONDecodeError) as
e:") so you can log e before returning False; for truly unexpected errors
re-raise them instead of swallowing (or let them propagate) so CI can surface
real failures. Ensure you replace each "except Exception:" with the specific
exception tuple relevant to the surrounding parse/validation logic and add
logging of the exception object.
Comment on lines
+121
to
+131
| # 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(): | ||
| 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 |
There was a problem hiding this comment.
Fork guard currently blocks valid pull_request_target comment flows.
Line 123 skips all fork PRs, including cases where the workflow runs on pull_request_target and has write permissions. That prevents the intended supported path.
✅ 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
Verify each finding against the current code and only fix it if needed.
In `@main.py` around lines 121 - 131, The current fork guard unconditionally
returns for any fork PR (is_fork_pr()), which also blocks valid
pull_request_target workflows; change the conditional so it only skips when the
event is a fork PR AND the workflow is NOT running under pull_request_target
(e.g., check os.getenv("GITHUB_EVENT_NAME") != "pull_request_target" or
equivalent), leaving the warning and return in the is_fork_pr() block but
permitting comment writes when the event is pull_request_target (optionally also
guard by an explicit writable token/permission check if available).
There was a problem hiding this comment.
Pull request overview
Updates the action’s PR-commenting behavior to better handle forked pull requests and permission-related API failures while reducing duplicate comment noise.
Changes:
- Add fork PR detection via
GITHUB_EVENT_PATHand short-circuit PR comment posting with a workflow warning. - Refactor PR comment management to update the latest matching comment and delete older duplicates.
- Add
GithubExceptionhandling (notably 403) with user-facing warnings and avoid failing the run due to comment-posting issues.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
main.py (2)
123-133:⚠️ Potential issue | 🔴 CriticalScope the fork skip to
pull_requestonly.Line 125 currently exits for all fork PRs, which also blocks valid
pull_request_targetcomment flows and undermines this PR’s objective.✅ Suggested 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
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 123 - 133, The current unconditional fork PR early return (the if is_fork_pr() block) blocks valid pull_request_target flows; narrow the check so it only triggers for the pull_request event. Update the condition around is_fork_pr() in main.py to also require the runtime event be "pull_request" (e.g., combine is_fork_pr() with an event-name check such as event == "pull_request" or use a helper like is_pull_request_event()), leaving pull_request_target and other events unaffected and keeping the same warning message and return behavior.
114-115:⚠️ Potential issue | 🟠 MajorAvoid blind
except Exceptionin control-path logic.Line 114 and Line 193 still swallow unexpected failures, which can hide real defects and make CI outcomes misleading.
✅ Suggested tightening
- except Exception: + except (OSError, json.JSONDecodeError, TypeError) as e: + print(f"::warning::Unable to parse GITHUB_EVENT_PATH: {e}", file=sys.stderr) return False ... - except Exception as e: - print(f"Error posting PR comment: {e}", file=sys.stderr) - return 0 + except (ValueError, TypeError) as e: + print(f"Error posting PR comment: {e}", file=sys.stderr) + return 0Also applies to: 193-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 114 - 115, The current blind "except Exception: return False" swallows all failures; replace it with targeted handling: catch the specific exceptions you expect (e.g., ValueError, KeyError, OSError) instead of Exception, or at minimum change to "except Exception as e", call the module logger (e.g., logger.exception or logger.error with e) to record the stack/message, and re-raise unexpected exceptions (or only return False for the expected exception types). Apply the same change to the other identical block that uses "except Exception: return False" so errors are logged and only known failure modes are suppressed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 161-176: The current logic builds matching_comments by filtering
comments only by body prefixes (using SUCCESS_TITLE/FAILURE_TITLE) and then
edits/deletes them; change this to also filter by the bot/automation author
before editing or deleting to avoid touching human comments: when creating
matching_comments, add an author check against the expected actor (e.g., compare
comment.user.login or comment.author to the bot/github app identity) so only
comments authored by your automation are included, then continue using
last_comment, pr_comment_body and the deletion loop safely for those filtered
results.
---
Duplicate comments:
In `@main.py`:
- Around line 123-133: The current unconditional fork PR early return (the if
is_fork_pr() block) blocks valid pull_request_target flows; narrow the check so
it only triggers for the pull_request event. Update the condition around
is_fork_pr() in main.py to also require the runtime event be "pull_request"
(e.g., combine is_fork_pr() with an event-name check such as event ==
"pull_request" or use a helper like is_pull_request_event()), leaving
pull_request_target and other events unaffected and keeping the same warning
message and return behavior.
- Around line 114-115: The current blind "except Exception: return False"
swallows all failures; replace it with targeted handling: catch the specific
exceptions you expect (e.g., ValueError, KeyError, OSError) instead of
Exception, or at minimum change to "except Exception as e", call the module
logger (e.g., logger.exception or logger.error with e) to record the
stack/message, and re-raise unexpected exceptions (or only return False for the
expected exception types). Apply the same change to the other identical block
that uses "except Exception: return False" so errors are logged and only known
failure modes are suppressed.
Comment on lines
+161
to
176
| 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() |
There was a problem hiding this comment.
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
Verify each finding against the current code and only fix it if needed.
In `@main.py` around lines 161 - 176, The current logic builds matching_comments
by filtering comments only by body prefixes (using SUCCESS_TITLE/FAILURE_TITLE)
and then edits/deletes them; change this to also filter by the bot/automation
author before editing or deleting to avoid touching human comments: when
creating matching_comments, add an author check against the expected actor
(e.g., compare comment.user.login or comment.author to the bot/github app
identity) so only comments authored by your automation are included, then
continue using last_comment, pr_comment_body and the deletion loop safely for
those filtered results.
Contributor
Author
|
Test result look good. No failed the CI anymore, and a warning message was added to the annotations. 
|
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 `@README.md`:
- Line 126: Replace the awkward sentence "For forked repositories skip PR
comment. For more details, refer to issue
[`#77`](https://github.com/commit-check/commit-check-action/issues/77)." with a
clearer phrasing and the correct issue link: update the text to something like
"Skip posting PR comments for forked repositories; see the fork-PR tracking
issue for details." and change the reference to the current fork-PR issue URL
(replace the [`#77`] reference with the correct issue number/URL). Ensure the new
wording appears where the original sentence string "For forked repositories skip
PR comment." is found and that the markdown link points to the fork-PR tracking
issue for this change.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #143
Summary by CodeRabbit
Bug Fixes
Documentation