From 3f95d8afcc0789ff5fbd7c4139413a5544508fb1 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Sun, 8 Feb 2026 23:57:16 -0500 Subject: [PATCH] some bugs --- .github/workflows/pr-review-status.yml | 53 +++++++++++++++++--- .github/workflows/pr-reviewer-assign.yml | 50 +++++++++++++------ .github/workflows/pr-reviewer-remind.yml | 63 ++++++++++++++++-------- 3 files changed, 124 insertions(+), 42 deletions(-) diff --git a/.github/workflows/pr-review-status.yml b/.github/workflows/pr-review-status.yml index e799332..a41d7a4 100644 --- a/.github/workflows/pr-review-status.yml +++ b/.github/workflows/pr-review-status.yml @@ -66,8 +66,15 @@ jobs: const prData = JSON.parse(fs.readFileSync(prFilePath, 'utf8')); - // Only care about the assigned main reviewer's reviews + // Check if reviewer is one of the assigned reviewers const isMainReviewer = reviewer.toLowerCase() === prData.main_reviewer.toLowerCase(); + const isSecondReviewer = prData.second_reviewer && + reviewer.toLowerCase() === prData.second_reviewer.toLowerCase(); + const isAssignedReviewer = isMainReviewer || isSecondReviewer; + + if (!isAssignedReviewer) { + console.log(`${reviewer} is not an assigned reviewer for PR #${prNumber}. Skipping state update.`); + } // ── Determine emoji and label ──────────────────────────────────── let emoji, label; @@ -99,8 +106,40 @@ jobs: } } - // ── Update the original Slack message with a status banner ─────── - if (prData.slack_thread_ts && isMainReviewer) { + // ── Check if all assigned reviewers are now done ───────────────── + // We need to query all reviews to know full status + let allReviews = []; + try { + const resp = await github.rest.pulls.listReviews({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }); + allReviews = resp.data; + } catch (err) { + console.log(`Could not fetch reviews: ${err.message}`); + } + + const REVIEW_STATES = ['APPROVED', 'CHANGES_REQUESTED', 'COMMENTED']; + const hasReviewed = (login) => allReviews.some( + r => r.user.login.toLowerCase() === login.toLowerCase() && REVIEW_STATES.includes(r.state) + ); + + const mainDone = hasReviewed(prData.main_reviewer); + const secondDone = !prData.second_reviewer || hasReviewed(prData.second_reviewer); + const allDone = mainDone && secondDone; + + // ── Update the original Slack message if all reviewers are done ── + if (prData.slack_thread_ts && allDone && isAssignedReviewer) { + // Build reviewer status lines + const reviewerLines = []; + const mainStatus = mainDone ? ':white_check_mark:' : ':hourglass_flowing_sand:'; + reviewerLines.push(`${mainStatus} *${prData.main_reviewer}* <@${prData.main_reviewer_slack}>`); + if (prData.second_reviewer) { + const secondStatus = secondDone ? ':white_check_mark:' : ':hourglass_flowing_sand:'; + reviewerLines.push(`${secondStatus} *${prData.second_reviewer}* <@${prData.second_reviewer_slack}>`); + } + await slackApi('chat.update', { channel: config.slack_channel_id, ts: prData.slack_thread_ts, @@ -117,7 +156,7 @@ jobs: type: 'section', text: { type: 'mrkdwn', - text: `:mag: *Main reviewer:* <@${prData.main_reviewer_slack}>\n:eyes: *Also reviewing:* <@${config.always_reviewer_slack}>`, + text: reviewerLines.join('\n'), }, }, ], @@ -137,12 +176,12 @@ jobs: }); } - // ── Update bot state if this is the main reviewer ──────────────── - if (isMainReviewer && prData.status === 'open') { + // ── Update bot state when all assigned reviewers are done ───────── + if (allDone && prData.status === 'open') { prData.status = 'reviewed'; prData.resolved_at = new Date().toISOString(); fs.writeFileSync(prFilePath, JSON.stringify(prData, null, 2) + '\n'); - console.log(`Marked PR #${prNumber} as reviewed`); + console.log(`All reviewers done — marked PR #${prNumber} as reviewed`); } - name: Commit and push bot-state diff --git a/.github/workflows/pr-reviewer-assign.yml b/.github/workflows/pr-reviewer-assign.yml index fcc5e20..be4314b 100644 --- a/.github/workflows/pr-reviewer-assign.yml +++ b/.github/workflows/pr-reviewer-assign.yml @@ -137,30 +137,33 @@ jobs: return; } - // ── Round-robin: pick next reviewer ────────────────────── + // ── Detect if author is the always-reviewer ───────────── + const authorSlackId = config.github_to_slack[prAuthor]; + const authorIsAlwaysReviewer = authorSlackId === config.always_reviewer_slack; + const reviewersNeeded = authorIsAlwaysReviewer ? 2 : 1; + + // ── Round-robin: pick reviewer(s) ───────────────────────── const roster = config.roster; let cursor = state.cursor || 0; - let mainReviewer = null; + const pickedReviewers = []; let newCursor = cursor; - for (let i = 0; i < roster.length; i++) { + for (let i = 0; i < roster.length && pickedReviewers.length < reviewersNeeded; i++) { const idx = (cursor + i) % roster.length; const candidate = roster[idx]; if (candidate.toLowerCase() !== prAuthor.toLowerCase()) { - mainReviewer = candidate; + pickedReviewers.push(candidate); newCursor = (idx + 1) % roster.length; - break; } } - // Fallback: if the only person in the roster is the author, - // assign the always-reviewer instead. - if (!mainReviewer) { + // Fallback: if we couldn't find enough reviewers + if (pickedReviewers.length === 0) { const alwaysEntry = Object.entries(config.github_to_slack).find( ([, slackId]) => slackId === config.always_reviewer_slack ); if (alwaysEntry) { - mainReviewer = alwaysEntry[0]; + pickedReviewers.push(alwaysEntry[0]); } else { core.setFailed('No eligible reviewer found and could not determine always-reviewer.'); return; @@ -168,13 +171,21 @@ jobs: newCursor = (cursor + 1) % roster.length; } + const mainReviewer = pickedReviewers[0]; + const secondReviewer = pickedReviewers.length > 1 ? pickedReviewers[1] : null; + const mainReviewerSlack = config.github_to_slack[mainReviewer]; if (!mainReviewerSlack) { core.setFailed(`No Slack ID mapping found for reviewer "${mainReviewer}".`); return; } + const secondReviewerSlack = secondReviewer ? config.github_to_slack[secondReviewer] : null; - console.log(`Assigning ${mainReviewer} to PR #${prNumber} (author: ${prAuthor}, cursor: ${cursor} → ${newCursor})`); + if (authorIsAlwaysReviewer) { + console.log(`Author is always-reviewer — assigning ${pickedReviewers.join(' + ')} to PR #${prNumber} (cursor: ${cursor} → ${newCursor})`); + } else { + console.log(`Assigning ${mainReviewer} to PR #${prNumber} (author: ${prAuthor}, cursor: ${cursor} → ${newCursor})`); + } // ── Request review on GitHub ───────────────────────────── try { @@ -182,14 +193,23 @@ jobs: owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber, - reviewers: [mainReviewer], + reviewers: pickedReviewers, }); - console.log(`Review requested from ${mainReviewer}`); + console.log(`Review requested from ${pickedReviewers.join(', ')}`); } catch (err) { - console.log(`Warning: could not request review from ${mainReviewer}: ${err.message}`); + console.log(`Warning: could not request reviewers: ${err.message}`); } // ── Post Slack notification ────────────────────────────── + let reviewerLines; + if (authorIsAlwaysReviewer && secondReviewer) { + // Author is the always-reviewer, so show two round-robin picks + reviewerLines = `:mag: *Reviewer 1:* <@${mainReviewerSlack}>\n:mag: *Reviewer 2:* <@${secondReviewerSlack}>`; + } else { + // Normal case: one main reviewer + always-reviewer + reviewerLines = `:mag: *Main reviewer:* <@${mainReviewerSlack}>\n:eyes: *Also reviewing:* <@${config.always_reviewer_slack}>`; + } + const slackTs = await postSlack({ channel: config.slack_channel_id, text: `New PR for review: <${prUrl}|#${prNumber} ${prTitle}>`, @@ -205,7 +225,7 @@ jobs: type: 'section', text: { type: 'mrkdwn', - text: `:mag: *Main reviewer:* <@${mainReviewerSlack}>\n:eyes: *Also reviewing:* <@${config.always_reviewer_slack}>`, + text: reviewerLines, }, }, ], @@ -222,6 +242,8 @@ jobs: author: prAuthor, main_reviewer: mainReviewer, main_reviewer_slack: mainReviewerSlack, + second_reviewer: secondReviewer, + second_reviewer_slack: secondReviewerSlack, created_at: new Date().toISOString(), last_reminded_at: null, slack_thread_ts: slackTs, diff --git a/.github/workflows/pr-reviewer-remind.yml b/.github/workflows/pr-reviewer-remind.yml index c6e2bb3..3c72ee6 100644 --- a/.github/workflows/pr-reviewer-remind.yml +++ b/.github/workflows/pr-reviewer-remind.yml @@ -38,7 +38,7 @@ jobs: const BOT_STATE_DIR = 'bot-state'; const configPath = path.join(BOT_STATE_DIR, 'config.json'); const prsDir = path.join(BOT_STATE_DIR, 'prs'); - const REMINDER_INTERVAL_MS = 48 * 60 * 60 * 1000; // 48 hours + const REMINDER_INTERVAL_MS = 24 * 60 * 60 * 1000; // 24 hours const CLEANUP_AGE_MS = 7 * 24 * 60 * 60 * 1000; // 7 days // ── Load config (Terraform-managed) ────────────────────── @@ -122,25 +122,29 @@ jobs: continue; } - // ── Check if main reviewer has submitted a review ────── - let hasReviewed = false; + // ── Check if reviewers have submitted reviews ────────── + let reviews = []; try { - const reviews = await github.rest.pulls.listReviews({ + const resp = await github.rest.pulls.listReviews({ owner: prOwner, repo: prRepo, pull_number: prData.pr_number, }); - hasReviewed = reviews.data.some( - r => - r.user.login.toLowerCase() === prData.main_reviewer.toLowerCase() && - ['APPROVED', 'CHANGES_REQUESTED', 'COMMENTED'].includes(r.state) - ); + reviews = resp.data; } catch (err) { console.log(`Could not fetch reviews for PR #${prData.pr_number}: ${err.message}`); } - if (hasReviewed) { - console.log(`PR #${prData.pr_number}: ${prData.main_reviewer} has reviewed. Done.`); + const REVIEW_STATES = ['APPROVED', 'CHANGES_REQUESTED', 'COMMENTED']; + const hasReviewed = (login) => reviews.some( + r => r.user.login.toLowerCase() === login.toLowerCase() && REVIEW_STATES.includes(r.state) + ); + + const mainDone = hasReviewed(prData.main_reviewer); + const secondDone = !prData.second_reviewer || hasReviewed(prData.second_reviewer); + + if (mainDone && secondDone) { + console.log(`PR #${prData.pr_number}: all assigned reviewers have reviewed. Done.`); prData.status = 'reviewed'; prData.resolved_at = new Date().toISOString(); fs.writeFileSync(filePath, JSON.stringify(prData, null, 2) + '\n'); @@ -148,12 +152,19 @@ jobs: continue; } - // ── Check if reviewer is still requested ─────────────── - const isStillRequested = ghPr.requested_reviewers.some( - r => r.login.toLowerCase() === prData.main_reviewer.toLowerCase() - ); - if (!isStillRequested) { - console.log(`PR #${prData.pr_number}: ${prData.main_reviewer} is no longer requested. Skipping reminder.`); + // ── Check if reviewers are still requested ───────────── + const requestedLogins = ghPr.requested_reviewers.map(r => r.login.toLowerCase()); + const pendingReviewers = []; + + if (!mainDone && requestedLogins.includes(prData.main_reviewer.toLowerCase())) { + pendingReviewers.push({ login: prData.main_reviewer, slack: prData.main_reviewer_slack }); + } + if (prData.second_reviewer && !secondDone && requestedLogins.includes(prData.second_reviewer.toLowerCase())) { + pendingReviewers.push({ login: prData.second_reviewer, slack: prData.second_reviewer_slack }); + } + + if (pendingReviewers.length === 0) { + console.log(`PR #${prData.pr_number}: no pending requested reviewers. Skipping reminder.`); continue; } @@ -169,11 +180,22 @@ jobs: } // ── Send reminder ────────────────────────────────────── - console.log(`PR #${prData.pr_number}: sending reminder for ${prData.main_reviewer}`); + const pendingNames = pendingReviewers.map(r => r.login).join(', '); + console.log(`PR #${prData.pr_number}: sending reminder for ${pendingNames}`); + + const mentionLines = pendingReviewers.map( + r => `:mag: <@${r.slack}> — please review when you get a chance.` + ); + // Only cc the always-reviewer if they aren't the PR author + // (if they are the author, they're not reviewing this one) + const authorSlackId = config.github_to_slack[prData.author]; + if (authorSlackId !== config.always_reviewer_slack) { + mentionLines.push(`:eyes: cc <@${config.always_reviewer_slack}>`); + } const reminderMsg = { channel: config.slack_channel_id, - text: `Reminder: PR #${prData.pr_number} awaits review from ${prData.main_reviewer}`, + text: `Reminder: PR #${prData.pr_number} awaits review from ${pendingNames}`, // Thread under the original assignment message when possible ...(prData.slack_thread_ts ? { thread_ts: prData.slack_thread_ts } : {}), blocks: [ @@ -184,8 +206,7 @@ jobs: text: [ `:bell: *Reminder:* <${prData.pr_url}|PR #${prData.pr_number}> is still awaiting review.`, ``, - `:mag: <@${prData.main_reviewer_slack}> — please review when you get a chance.`, - `:eyes: cc <@${config.always_reviewer_slack}>`, + ...mentionLines, ].join('\n'), }, },