Filter PRs by user in isFirstPullRequest - #402
Conversation
The isFirstPullRequest function was checking ALL PRs in the repo instead of only the current user's PRs. This caused everyone to be identified as a new contributor in repos with issues disabled (since isFirstIssue returns true when no issues exist). Now isFirstPullRequest filters PRs by user login, matching the behavior of isFirstIssue. Closes actions#369 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
For issue events, only check isFirstIssue(). For PR events, only check isFirstPullRequest(). This fixes the case where repos with issues disabled would mark everyone as a new contributor (since isFirstIssue returns true when no issues exist). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Hi, just checking in on this PR—happy to make any changes or provide more context if helpful. |
|
Related: #410 |
|
@ncalteen Hiya, just checking in on this PR—happy to make any changes or provide more context if helpful. |
| // Check if this is the user's first contribution of the relevant type. | ||
| if (isIssue && !(await isFirstIssue(octokit))) | ||
| return core.info('Skipping...Not First Issue') | ||
| if (isPullRequest && !(await isFirstPullRequest(octokit))) | ||
| return core.info('Skipping...Not First Pull Request') |
There was a problem hiding this comment.
Checking typed contributions is a valid use case, but so is checking untyped contributions, I think. Maybe an issue comment should not be created if a user is familiar enough with the repository to have authored a PR.
In my opinion the name of the action first-interaction implies any contribution. The action could have subdirectories like actions/first-interaction/first-pr and actions/first-interaction/first-issue for checking specific types.
| // Check if this is the user's first contribution of the relevant type. | |
| if (isIssue && !(await isFirstIssue(octokit))) | |
| return core.info('Skipping...Not First Issue') | |
| if (isPullRequest && !(await isFirstPullRequest(octokit))) | |
| return core.info('Skipping...Not First Pull Request') | |
| // Check if this is the user's first contribution. | |
| if (!(await isFirstIssue(octokit) && await isFirstPullRequest(octokit))) | |
| return core.info('Skipping...Not First Contribution') |
Though, maybe clearer function names would be something like userHasAuthoredPullRequestsPreviously and userHasReportedIssuesPreviously.
There was a problem hiding this comment.
Adopted in eae58ce — agreed, and the naming argument is the deciding one. first-interaction should mean any contribution.
The committed line differs from your suggestion by two pairs of parentheses:
if (!((await isFirstIssue(octokit)) && (await isFirstPullRequest(octokit))))
return core.info('Skipping...Not First Contribution')That is prettier's doing rather than a change of intent — npm run lint rejects the suggestion as written:
src/main.ts
41:9 error Replace `await·isFirstIssue(octokit)·&&·await·isFirstPullRequest(octokit` with `(await·isFirstIssue(octokit))·&&·(await·isFirstPullRequest(octokit)` prettier/prettier
and running npx prettier src/main.ts over your version emits the line above verbatim. await already binds tighter than &&, so the parentheses are cosmetic — same expression, same precedence.
The earlier commit in this PR, e73de6f, is still needed underneath it. On main, isFirstPullRequest never filters by author:
return (
// Filter out any PRs that are newer than the current one.
pulls.filter((pull) => pull.number < github.context.issue.number)
.length === 0
)pulls.list is called with only owner, repo and state — there is no creator parameter for pulls — and nothing filters afterward, so this returns true only when the incoming PR is the lowest-numbered PR in the repo. That's the isFirstPullRequest(octokit) half of your condition, so it had to be fixed for the combined check to mean anything.
eae58ce also adds the test the change is really about, which nothing covered before:
✓ Skips a PR message if the sender has only prior issues
One thing worth a maintainer's eye: this is a change of behaviour, not a restoration. Before #311 the action branched on event type —
if (isIssue) {
firstContribution = await isFirstIssue(client, …);
} else {
firstContribution = await isFirstPull(client, …);
}— and that isFirstPull did filter by author, under the comment // No way to filter pulls by creator. So #311 dropped the branching and the author filter together, and #410 proposes restoring the branching. I've gone with your reading because the name supports it, but it is a new semantic rather than a revert.
On naming: agreed the current ones are ambiguous, though the rename inverts polarity at every call site — probably its own PR.
actions/first-interaction@v3.1.0 (固定SHA: 1c4688942c71f71d4f5502a26ea67c331730fa4d) には
未修正の上流バグが2つあり、ワークフローが機能しない状態だった。
1. with: の入力キーがハイフン区切り (repo-token 等) だが、固定SHA時点の実装は
core.getInput('repo_token', ...) のようにアンダースコア区切りで読み取るため、
ランナーが生成する環境変数名 (INPUT_REPO-TOKEN) と一致せず、required な入力が
見つからずジョブが常に失敗する。
2. isFirstPullRequest() がリポジトリ全体のPR番号のみで判定しており投稿者を
区別しないため、リポジトリの最初のPRより後は誰の初回PRでも「初回ではない」
と誤判定される(actions/first-interaction#402 で未修正のまま報告済み)。
third-party action への依存をやめ、gh api / gh issue comment / gh pr comment で
投稿者ごとのIssue/PR件数を数える最小実装に置き換えた。
レビューコメント: #189 (comment)
レビューコメント: #189 (comment)
レビュアー: coderabbitai
優先度: high
Adopt the review suggestion on actions#402: instead of checking only the contribution type that raised the event, require that this is both the sender's first issue and their first pull request. The action is named first-interaction, which reads as any contribution — someone who has already opened an issue is familiar enough with the repo that a first-PR greeting is noise. This still fixes actions#369: in a repo with issues disabled isFirstIssue is vacuously true, so the combined check falls through to the pull request check instead of short-circuiting into greeting everyone. Note this is a change of behaviour rather than a restoration; the pre-actions#311 implementation branched on event type. Co-Authored-By: LapNik <73829722+LapNik@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the PR description and tests were updated to validate the corrected first-contribution behavior.
Pull request overview
This pull request fixes first-contribution detection for the first-interaction action by (1) making the “first PR” check specific to the PR author and (2) changing the greeting behavior to require the sender to have no prior issues and no prior pull requests.
Changes:
- Update
run()to greet only when bothisFirstIssue()andisFirstPullRequest()are true. - Update
isFirstPullRequest()to ignore pull requests created by other users. - Update/add tests to cover author filtering and cross-type (“prior issues”) cases; rebuild
dist/.
File summaries
| File | Description |
|---|---|
| src/main.ts | Adjusts first-contribution gating logic and filters PR history by sender login. |
| dist/index.js | Rebuilt bundled output reflecting the updated logic. |
| tests/main.test.ts | Expands coverage for author-filtering and any-contribution semantics. |
Review details
- Files reviewed: 2/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
Two changes, both needed for first-contribution detection to be correct:
isFirstPullRequest()now filters by author.pulls.listhas nocreatorparameter and nothing filtered afterward, so the check returnedtrueonly when the incoming PR was the lowest-numbered PR in the repo's entire history — any prior PR by anyone disqualified a genuine first-time contributor.The first-contribution check now covers any contribution type. The action greets only when this is both the sender's first issue and their first pull request:
first-interactionreads as any contribution — someone who has already opened an issue is familiar enough with the repo that a first-PR greeting is noise.Why this fixes #369
On
mainthe check is!isFirstIssue && !isFirstPullRequest. In a repo with issues disabled,GET /repos/{owner}/{repo}/issuesreturns200and serves pull requests only, so after thepull_request === undefinedfilterisFirstIssueis unconditionallytrue. That short-circuits the&&, and the action greets every PR author forever.Under the combined check a vacuously-true
isFirstIssuesimply falls through to the pull request check, which now filters by author.Note for reviewers
This is a change of behaviour, not a restoration. The pre-#311 implementation branched on event type, and its
isFirstPullfiltered by author under the comment// No way to filter pulls by creator; #311 dropped both. Restoring the branching alone fixes the over-greeting but leaves the author filter missing, which turns over-greeting into never-greeting. The any-contribution reading here is a deliberate choice, adopted from review on this PR.Test plan
Fixes #369.