Skip to content

Fix incorrect first-interaction detection#410

Open
t-hamano wants to merge 4 commits into
actions:mainfrom
t-hamano:fix/first-contribution-event-branching
Open

Fix incorrect first-interaction detection#410
t-hamano wants to merge 4 commits into
actions:mainfrom
t-hamano:fix/first-contribution-event-branching

Conversation

@t-hamano

@t-hamano t-hamano commented Jun 20, 2026

Copy link
Copy Markdown

Problem

This PR fixes an issue where the action incorrectly detects a "first interaction" and greets contributors who are not actually first-time contributors.

This conditional statement was modified by #311, which appears to be unintentional.

Before:

let firstContribution = false;
if (isIssue) {
    firstContribution = yield isFirstIssue(client, issue.owner, issue.repo, sender, issue.number);
}
else {
    firstContribution = yield isFirstPull(client, issue.owner, issue.repo, sender, issue.number);
}
if (!firstContribution) {
    console.log('Not the users first contribution');
    return;
}

https://github.com/actions/first-interaction/pull/311/changes#diff-7934bf411fea192ad8cd69e0a12911648a2842cb0f2409a8fb67b41b7069d757L62-L67

After:

if (!(await isFirstIssue(octokit)) && !(await isFirstPullRequest(octokit)))
    return core.info('Skipping...Not First Contribution')

https://github.com/actions/first-interaction/pull/311/changes#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3dR46

This logic change will cause isFirstIssue to always be true if the issue feature is disabled on the repository, consequently adding a message to contributors who have already submitted a PR.

Approach

Restore the original behavior: evaluate only the check matching the event type. As a result, both the first issue and the first pull request are considered first contributions. This is the same specification as before the action was converted from a container action.

Evaluate only the check matching the event type when deciding whether
this is the sender's first contribution: an issue event looks at issue
history, a PR event at pull request history.

PR actions#311 (TypeScript conversion) replaced the original event-branching
logic with a combined `isFirstIssue() && isFirstPullRequest()` condition,
which posts the greeting when *either* is the sender's first. When issues
are disabled, isFirstIssue() always returns true, so every PR (even from
repeat contributors) was greeted. The symmetric case (a sender with no
PRs always greeted on issues) was also broken.

Add regression tests covering both directions.

Co-Authored-By: Claude <noreply@anthropic.com>
@t-hamano t-hamano changed the title Fix first-interaction misfiring when issues are disabled Fix incorrect first-interaction detection introduced in #311 Jun 20, 2026
@t-hamano t-hamano changed the title Fix incorrect first-interaction detection introduced in #311 Fix incorrect first-interaction detection Jun 20, 2026
@t-hamano t-hamano marked this pull request as ready for review June 20, 2026 14:46
@t-hamano t-hamano requested a review from a team as a code owner June 20, 2026 14:46
Copilot AI review requested due to automatic review settings June 20, 2026 14:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR restores the original “first interaction” semantics by checking only the contribution type that matches the triggering event (issue vs pull request), preventing repeat contributors from being incorrectly greeted when only one feature (issues/PRs) is enabled.

Changes:

  • Update first-contribution detection to evaluate only isFirstIssue for issue events and only isFirstPullRequest for PR events.
  • Regenerate dist/index.js to reflect the source change.
  • Expand/adjust unit tests to cover event-specific first-contribution scenarios.

Reviewed changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/main.ts Restores event-specific first-interaction detection logic (issue vs PR).
dist/index.js Updates the built artifact to match src/ behavior changes.
__tests__/main.test.ts Adds/updates tests for event-specific skipping behavior (but needs tightening to catch regressions).
Comments suppressed due to low confidence (1)

tests/main.test.ts:96

  • This test now describes “prior issues and PRs”, but run() only evaluates the first-contribution check that matches the event type (issue vs PR). Because the default test context is a PR event, the current setup (mocking two paginate calls) can still pass even if a regression reintroduces calling both checks. Rename the test to match the PR-only behavior and assert the correct API call/count.
    it('Skips a message if the sender has prior issues and PRs', async () => {
      mocktokit.paginate
        // Issues
        .mockResolvedValueOnce([
          {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread __tests__/main.test.ts
Comment thread __tests__/main.test.ts
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.

Comment thread __tests__/main.test.ts Outdated
Comment thread __tests__/main.test.ts Outdated
Comment thread __tests__/main.test.ts
t-hamano and others added 2 commits June 20, 2026 23:54
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Address review feedback: the PR-event regression test mocked an unused
issues paginate call and only checked the log message. Remove the unused
mock and assert that paginate is called exactly once with pulls.list, so
the test fails if both checks are accidentally reintroduced for PR events.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants