diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 580d711149f..1d5b8d54f4e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -48,6 +48,7 @@ jobs: \.github/workflows/labeler-*.yml \.github/workflows/markdownlint*.yml \.github/workflows/refresh-manifests.yml + \.github/workflows/pr-review-needed.yml \.github/workflows/specialized-test-runner.yml \.github/workflows/tests-outerloop.yml \.github/workflows/tests-quarantine.yml diff --git a/.github/workflows/pr-review-needed.yml b/.github/workflows/pr-review-needed.yml index 1d3551e4b20..0fdbbee80f7 100644 --- a/.github/workflows/pr-review-needed.yml +++ b/.github/workflows/pr-review-needed.yml @@ -2,16 +2,20 @@ # # A PR is considered "needing review" when: # - It is not a draft +# - It does not have the NO-MERGE label # - It has not been approved (or approval was superseded by changes requested/dismissed) # - The last activity (comment, review, or commit) was from the PR author or prompter (for bot PRs) # # The workflow runs on demand and updates issue #13834 with a table of PRs needing review, -# sorted by last updated date (most recent first). +# sorted by waiting time (oldest first). -name: Update PRs Needing Review +name: PRs Need Feedback on: workflow_dispatch: + schedule: + # Runs every 2 hours + - cron: '0 */2 * * *' permissions: issues: write @@ -29,14 +33,31 @@ jobs: uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 with: script: | + // Set to true to enable debug logging for date calculations + const DEBUG_LOGGING = false; + + // Helper function to handle pagination and get all data + async function getAllPages(method, params) { + const results = []; + let page = 1; + while (true) { + const response = await method({ ...params, per_page: 100, page }); + results.push(...response.data); + if (response.data.length < 100) { + break; + } + page++; + } + return results; + } + // Get all open PRs - const { data: pullRequests } = await github.rest.pulls.list({ + const pullRequests = await getAllPages(github.rest.pulls.list, { owner: context.repo.owner, repo: context.repo.repo, state: 'open', sort: 'updated', - direction: 'desc', - per_page: 1000 + direction: 'desc' }); const prsNeedingReview = []; @@ -47,8 +68,38 @@ jobs: continue; } + // Skip PRs created by bots + if (pr.user.login.endsWith('[bot]')) { + continue; + } + + // Skip PRs with NO-MERGE label + if (pr.labels.some(label => label.name === 'NO-MERGE')) { + continue; + } + + // Fetch the full PR details to get mergeable_state (not available in list endpoint) + const { data: fullPr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number + }); + + // Check if PR has merge conflicts + // mergeable_state can be: clean, dirty, blocked, behind, unstable, unknown, or null + // - 'dirty' definitely means conflicts + // - mergeable === false + not blocked/behind/unstable also likely means conflicts + // - blocked/behind/unstable mean the PR can't merge due to other reasons (checks, needs update, etc.) + const hasMergeConflict = fullPr.mergeable_state === 'dirty' || + (fullPr.mergeable === false && + fullPr.mergeable_state !== 'blocked' && + fullPr.mergeable_state !== 'behind' && + fullPr.mergeable_state !== 'unstable' && + fullPr.mergeable_state !== 'unknown' && + fullPr.mergeable_state !== 'clean'); + // Get reviews to check if PR is approved - const { data: reviews } = await github.rest.pulls.listReviews({ + const reviews = await getAllPages(github.rest.pulls.listReviews, { owner: context.repo.owner, repo: context.repo.repo, pull_number: pr.number @@ -99,110 +150,198 @@ jobs: cannotApprove.add(prompter.toLowerCase()); } - // Get the last activity - check both comments and commits - let lastExternalActivity = null; - let lastExternalUser = null; - // Get issue comments (regular comments) - const { data: comments } = await github.rest.issues.listComments({ + const comments = await getAllPages(github.rest.issues.listComments, { owner: context.repo.owner, repo: context.repo.repo, - issue_number: pr.number, - per_page: 1000 + issue_number: pr.number }); // Get review comments (inline code comments) - const { data: reviewComments } = await github.rest.pulls.listReviewComments({ + const reviewComments = await getAllPages(github.rest.pulls.listReviewComments, { owner: context.repo.owner, repo: context.repo.repo, - pull_number: pr.number, - per_page: 1000 + pull_number: pr.number }); // Get commits to check the last pusher - const { data: commits } = await github.rest.pulls.listCommits({ + const commits = await getAllPages(github.rest.pulls.listCommits, { owner: context.repo.owner, repo: context.repo.repo, - pull_number: pr.number, - per_page: 1000 + pull_number: pr.number }); - // Check comments for last external activity - for (const comment of comments) { - const commentUser = comment.user.login.toLowerCase(); - if (!cannotApprove.has(commentUser)) { - const commentDate = new Date(comment.created_at); - if (!lastExternalActivity || commentDate > lastExternalActivity) { - lastExternalActivity = commentDate; - lastExternalUser = comment.user.login; - } - } - } + // Get timeline events to find when PR was marked ready for review + const timelineEvents = await getAllPages(github.rest.issues.listEventsForTimeline, { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pr.number + }); - // Check review comments - for (const comment of reviewComments) { - const commentUser = comment.user.login.toLowerCase(); - if (!cannotApprove.has(commentUser)) { - const commentDate = new Date(comment.created_at); - if (!lastExternalActivity || commentDate > lastExternalActivity) { - lastExternalActivity = commentDate; - lastExternalUser = comment.user.login; - } + // Find the last "ready_for_review" event, or use PR creation date + let readyForReviewDate = new Date(pr.created_at); + for (const event of timelineEvents) { + if (event.event === 'ready_for_review') { + // Use most recent ready for review event date. + readyForReviewDate = new Date(event.created_at); } } - // Check reviews (for review submissions) - for (const review of reviews) { - const reviewUser = review.user.login.toLowerCase(); - if (!cannotApprove.has(reviewUser) && review.submitted_at) { - const reviewDate = new Date(review.submitted_at); - if (!lastExternalActivity || reviewDate > lastExternalActivity) { - lastExternalActivity = reviewDate; - lastExternalUser = review.user.login; - } + if (DEBUG_LOGGING) { + console.log(`\n=== PR #${pr.number}: ${pr.title} ===`); + console.log(`Author: ${prAuthor}, Prompter: ${prompter || 'none'}`); + console.log(`Cannot approve: ${Array.from(cannotApprove).join(', ')}`); + console.log(`Created: ${pr.created_at}, Ready for review: ${readyForReviewDate.toISOString()}`); + console.log(`Timeline events (${timelineEvents.length} total):`); + for (const event of timelineEvents) { + console.log(` ${event.event} at ${event.created_at || 'no date'}`); } } - // Track the last activity from someone who CAN'T approve (author/prompter) - let lastInternalActivity = null; + // Collect all internal (author/prompter) and external (reviewers) activities + const internalActivities = []; + const externalActivities = []; + + // Helper to check if a user is Copilot or a bot we should ignore + const isCopilotOrBot = (login) => { + const lowerLogin = login.toLowerCase(); + return lowerLogin === 'copilot' || lowerLogin.endsWith('[bot]'); + }; - // Check commits for activity from author/prompter - if (commits.length > 0) { - const lastCommit = commits[commits.length - 1]; - const commitAuthor = lastCommit.author?.login?.toLowerCase(); - const commitCommitter = lastCommit.committer?.login?.toLowerCase(); - const commitDate = new Date(lastCommit.commit.committer.date); + // Process commits + for (const commit of commits) { + const commitAuthor = commit.author?.login?.toLowerCase(); + const commitCommitter = commit.committer?.login?.toLowerCase(); + const commitDate = new Date(commit.commit.committer.date); + + // Skip Copilot activities + if ((commitAuthor && isCopilotOrBot(commitAuthor)) || (commitCommitter && isCopilotOrBot(commitCommitter))) { + if (DEBUG_LOGGING) { + console.log(` Commit ${commit.sha.substring(0, 7)} (${commitDate.toISOString()}): SKIPPED (Copilot)`); + } + continue; + } const isAuthorInternal = commitAuthor && cannotApprove.has(commitAuthor); const isCommitterInternal = commitCommitter && cannotApprove.has(commitCommitter); if (isAuthorInternal || isCommitterInternal) { - lastInternalActivity = commitDate; + internalActivities.push(commitDate); + if (DEBUG_LOGGING) { + console.log(` Commit ${commit.sha.substring(0, 7)} (${commitDate.toISOString()}): INTERNAL (author: ${commitAuthor}, committer: ${commitCommitter})`); + } + } else { + externalActivities.push(commitDate); + if (DEBUG_LOGGING) { + console.log(` Commit ${commit.sha.substring(0, 7)} (${commitDate.toISOString()}): EXTERNAL (author: ${commitAuthor}, committer: ${commitCommitter})`); + } } + // Commits from users outside cannotApprove are treated as external activities } - // Check comments from author/prompter + // Process issue comments for (const comment of comments) { const commentUser = comment.user.login.toLowerCase(); + + // Skip Copilot activities + if (isCopilotOrBot(commentUser)) { + if (DEBUG_LOGGING) { + console.log(` Comment by ${commentUser} (${comment.created_at}): SKIPPED (Copilot)`); + } + continue; + } + + const commentDate = new Date(comment.created_at); if (cannotApprove.has(commentUser)) { - const commentDate = new Date(comment.created_at); - if (!lastInternalActivity || commentDate > lastInternalActivity) { - lastInternalActivity = commentDate; + internalActivities.push(commentDate); + if (DEBUG_LOGGING) { + console.log(` Comment by ${commentUser} (${comment.created_at}): INTERNAL`); + } + } else { + externalActivities.push(commentDate); + if (DEBUG_LOGGING) { + console.log(` Comment by ${commentUser} (${comment.created_at}): EXTERNAL`); } } } - // Check review comments from author/prompter + // Process review comments for (const comment of reviewComments) { const commentUser = comment.user.login.toLowerCase(); + + // Skip Copilot activities + if (isCopilotOrBot(commentUser)) { + if (DEBUG_LOGGING) { + console.log(` Review comment by ${commentUser} (${comment.created_at}): SKIPPED (Copilot)`); + } + continue; + } + + const commentDate = new Date(comment.created_at); if (cannotApprove.has(commentUser)) { - const commentDate = new Date(comment.created_at); - if (!lastInternalActivity || commentDate > lastInternalActivity) { - lastInternalActivity = commentDate; + internalActivities.push(commentDate); + if (DEBUG_LOGGING) { + console.log(` Review comment by ${commentUser} (${comment.created_at}): INTERNAL`); + } + } else { + externalActivities.push(commentDate); + if (DEBUG_LOGGING) { + console.log(` Review comment by ${commentUser} (${comment.created_at}): EXTERNAL`); + } + } + } + + // Process reviews + for (const review of reviews) { + const reviewUser = review.user.login.toLowerCase(); + + // Skip Copilot activities + if (isCopilotOrBot(reviewUser)) { + if (DEBUG_LOGGING) { + console.log(` Review by ${reviewUser} (${review.submitted_at}): SKIPPED (Copilot)`); + } + continue; + } + + if (!cannotApprove.has(reviewUser) && review.submitted_at) { + externalActivities.push(new Date(review.submitted_at)); + if (DEBUG_LOGGING) { + console.log(` Review by ${reviewUser} (${review.submitted_at}): EXTERNAL (state: ${review.state})`); + } + } else if (DEBUG_LOGGING) { + console.log(` Review by ${reviewUser} (${review.submitted_at}): IGNORED (cannotApprove or no date)`); + } + } + + // Find last activity for each group + const lastInternalActivity = internalActivities.length > 0 + ? internalActivities.reduce((a, b) => a > b ? a : b) + : null; + const lastExternalActivity = externalActivities.length > 0 + ? externalActivities.reduce((a, b) => a > b ? a : b) + : null; + + // Find the first internal activity after the last external activity + let firstInternalAfterExternal = null; + if (internalActivities.length > 0 && lastExternalActivity) { + const sortedInternalActivities = [...internalActivities].sort((a, b) => a - b); + for (const date of sortedInternalActivities) { + if (date > lastExternalActivity) { + firstInternalAfterExternal = date; + break; } } } + if (DEBUG_LOGGING) { + console.log(`Summary:`); + console.log(` Internal activities: ${internalActivities.length}`); + console.log(` External activities: ${externalActivities.length}`); + console.log(` Last internal: ${lastInternalActivity ? lastInternalActivity.toISOString() : 'none'}`); + console.log(` Last external: ${lastExternalActivity ? lastExternalActivity.toISOString() : 'none'}`); + console.log(` First internal after external: ${firstInternalAfterExternal ? firstInternalAfterExternal.toISOString() : 'none'}`); + } + // Determine if the PR needs review: // The last activity should be from someone who can't approve (author/prompter) // meaning the ball is in the reviewers' court @@ -218,19 +357,48 @@ jobs: // If only external activity or no activity at all, doesn't need review if (needsReview) { + // Check if PR has the community-contribution label + const isCommunityContribution = pr.labels.some(label => label.name === 'community-contribution'); + + // Determine the effective author - for Copilot PRs, use the first non-Copilot assignee + let effectiveAuthor = pr.user.login; + let effectiveAuthorUrl = pr.user.html_url; + + if (pr.user.login.toLowerCase() === 'copilot' || pr.user.login.toLowerCase() === 'copilot[bot]') { + const nonCopilotAssignee = pr.assignees.find(a => { + const assigneeLogin = a.login.toLowerCase(); + return assigneeLogin !== 'copilot' && assigneeLogin !== 'copilot[bot]'; + }); + if (nonCopilotAssignee) { + effectiveAuthor = nonCopilotAssignee.login; + effectiveAuthorUrl = nonCopilotAssignee.html_url; + } + } + + const updatedAt = firstInternalAfterExternal ? firstInternalAfterExternal.toISOString() : readyForReviewDate.toISOString(); + + if (DEBUG_LOGGING) { + console.log(` Needs review: YES`); + console.log(` Updated at: ${updatedAt} (${firstInternalAfterExternal ? 'first internal after external' : 'ready for review date'})`); + } + prsNeedingReview.push({ number: pr.number, title: pr.title, - author: pr.user.login, + author: effectiveAuthor, url: pr.html_url, - authorUrl: pr.user.html_url, - updatedAt: pr.updated_at + authorUrl: effectiveAuthorUrl, + updatedAt: updatedAt, + isCommunityContribution: isCommunityContribution, + hasMergeConflict: hasMergeConflict }); + } else if (DEBUG_LOGGING) { + console.log(` Needs review: NO`); } } - // Sort by updated date descending - prsNeedingReview.sort((a, b) => new Date(b.updatedAt) - new Date(a.updatedAt)); + // Sort by updated date ascending (oldest first) + prsNeedingReview.sort((a, b) => new Date(a.updatedAt) - new Date(b.updatedAt)); console.log('Found ' + prsNeedingReview.length + ' PRs needing review'); core.setOutput('prs', JSON.stringify(prsNeedingReview)); @@ -241,6 +409,8 @@ jobs: steps: - name: Update tracking issue uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + env: + PRS_JSON: ${{ needs.get-prs-needing-review.outputs.prs }} with: script: | // Issue number to update with the PR list @@ -249,8 +419,8 @@ jobs: // Unique marker to identify the automated content const marker = ''; - // Get PRs from previous job - const prsNeedingReview = JSON.parse('${{ needs.get-prs-needing-review.outputs.prs }}'); + // Get PRs from previous job (via environment variable to avoid string interpolation issues) + const prsNeedingReview = JSON.parse(process.env.PRS_JSON); // Format time since last update function formatTimeSince(dateStr) { @@ -261,13 +431,43 @@ jobs: const diffHours = Math.floor(diffMs / (1000 * 60 * 60)); const diffDays = Math.floor(diffMs / (1000 * 60 * 60 * 24)); + // Determine circle color based on age + let circle; + if (diffDays > 3) { + circle = '🔴'; + } else if (diffDays > 1) { + circle = '🟡'; + } else { + circle = '🟢'; + } + + let timeText; if (diffDays > 0) { - return diffDays + ' day' + (diffDays === 1 ? '' : 's') + ' ago'; + timeText = diffDays + ' day' + (diffDays === 1 ? '' : 's') + ' ago'; } else if (diffHours > 0) { - return diffHours + ' hour' + (diffHours === 1 ? '' : 's') + ' ago'; + timeText = diffHours + ' hour' + (diffHours === 1 ? '' : 's') + ' ago'; } else { - return diffMins + ' minute' + (diffMins === 1 ? '' : 's') + ' ago'; + timeText = diffMins + ' minute' + (diffMins === 1 ? '' : 's') + ' ago'; + } + + return circle + ' ' + timeText; + } + + // Function to generate a PR table + function generatePRTable(prs, emptyMessage) { + if (prs.length === 0) { + return '_' + emptyMessage + '_\n\n'; } + let table = '| PR | Author | Waiting for feedback |\n'; + table += '|---|---|---|\n'; + for (const pr of prs) { + const authorLink = '[' + pr.author + '](' + pr.authorUrl + ')'; + const conflictEmoji = pr.hasMergeConflict ? ' ❌' : ''; + const prLink = '[#' + pr.number + ' - ' + pr.title + '](' + pr.url + ')' + conflictEmoji; + const timeSince = formatTimeSince(pr.updatedAt); + table += '| ' + prLink + ' | ' + authorLink + ' | ' + timeSince + ' |\n'; + } + return table + '\n'; } // Build the table @@ -275,21 +475,28 @@ jobs: tableContent += '## PRs Needing Review\n\n'; tableContent += '_Last updated: ' + new Date().toISOString() + '_\n\n'; - if (prsNeedingReview.length === 0) { - tableContent += '🎉 No PRs currently need review!\n'; - } else { - tableContent += '| PR | Author | Last Updated |\n'; - tableContent += '|---|---|---|\n'; - for (const pr of prsNeedingReview) { - // Use display name without @ to avoid notifications - const authorLink = '[' + pr.author + '](' + pr.authorUrl + ')'; - const prLink = '[#' + pr.number + ' - ' + pr.title + '](' + pr.url + ')'; - const timeSince = formatTimeSince(pr.updatedAt); - tableContent += '| ' + prLink + ' | ' + authorLink + ' | ' + timeSince + ' |\n'; - } - } + // Split PRs into community and team PRs + const communityPRs = prsNeedingReview.filter(pr => pr.isCommunityContribution); + const teamPRs = prsNeedingReview.filter(pr => !pr.isCommunityContribution); + + // Team PRs table + tableContent += '### Team PRs\n\n'; + tableContent += generatePRTable(teamPRs, 'No team PRs currently need review.'); - tableContent += '\n_This list is automatically updated every hour._'; + // Community PRs table + tableContent += '### Community Contributions\n\n'; + tableContent += generatePRTable(communityPRs, 'No community PRs currently need review.'); + + tableContent += '\n_This list is automatically updated every 2 hours._'; + + // If this is a manual workflow_dispatch run, just output the markdown to console + if (context.eventName === 'workflow_dispatch') { + console.log('=== Manual workflow run - Issue body (not updating issue) ==='); + console.log(tableContent); + console.log('=== End of issue body ==='); + console.log('Found ' + prsNeedingReview.length + ' PRs needing review (issue not updated for manual runs)'); + return; + } // Get the current issue const { data: issue } = await github.rest.issues.get({