From d61b0fb94615797afd59c47ef33a022a5f5f81c8 Mon Sep 17 00:00:00 2001 From: Maaren <80040706+SSoggyTacoMan@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:26:49 +0200 Subject: [PATCH 1/3] fix(ci): preserve review states across empty and merge commits without game changes --- .github/scripts/auto-triage.mjs | 27 +++++- .github/scripts/review-state.mjs | 9 +- .github/scripts/review-state.test.mjs | 121 ++++++++++++++++++++++++- .github/scripts/review-utils.mjs | 23 +++++ .github/scripts/stale-review-queue.mjs | 47 ++++++++-- .github/scripts/sync-reviews.mjs | 32 +++++++ 6 files changed, 245 insertions(+), 14 deletions(-) diff --git a/.github/scripts/auto-triage.mjs b/.github/scripts/auto-triage.mjs index 82b36865bc..234aca7044 100644 --- a/.github/scripts/auto-triage.mjs +++ b/.github/scripts/auto-triage.mjs @@ -14,6 +14,7 @@ import { githubRequest, hasLabel, inferSubmissionMetadata, + isReviewCommitOutdated, parseTags, readGitHubEvent, removeLabel, @@ -523,11 +524,32 @@ function readFileSafe(filePath) { } } -async function getLatestReviewStatus({ owner, repo, token, prNumber, reviewers, authorLogin, headSha }) { +async function getLatestReviewStatus({ owner, repo, token, prNumber, reviewers, authorLogin, headSha, pullFiles }) { if (!reviewers || reviewers.size === 0) return "unknown"; try { const reviews = await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/reviews`); - return reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha }); + const outdatedCommitIds = new Set(); + for (const r of reviews) { + if (r.commit_id && r.commit_id !== headSha) { + const isOutdated = await isReviewCommitOutdated({ + owner, + repo, + token, + prNumber, + commitId: r.commit_id, + headSha, + submissionFiles: pullFiles, + }); + if (isOutdated) outdatedCommitIds.add(r.commit_id); + } + } + return reconcileReviewStatus({ + reviews, + reviewers, + authorLogin, + headSha, + isCommitOutdated: (cid) => cid !== headSha && outdatedCommitIds.has(cid), + }); } catch (e) { console.warn("Unable to fetch reviews; skipping review reconciliation:", e.message); return "unknown"; @@ -545,6 +567,7 @@ async function applyLabels(result) { reviewers, authorLogin: pullRequest.user?.login, headSha: pullRequest.head?.sha, + pullFiles, }); const changes = autoReviewLabelChanges({ labels: currentLabels, diff --git a/.github/scripts/review-state.mjs b/.github/scripts/review-state.mjs index 475ceeac45..43c4daeac8 100644 --- a/.github/scripts/review-state.mjs +++ b/.github/scripts/review-state.mjs @@ -47,11 +47,14 @@ export function autoReviewLabelChanges({ labels, validationOk, eventAction, revi } } -export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha }) { +export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha, isCommitOutdated }) { if (!reviewers || reviewers.size === 0) return "unknown"; const lowerReviewers = new Set([...reviewers].map((u) => u.toLowerCase())); const lowerAuthor = authorLogin?.toLowerCase(); + // ponytail: default considers a commit outdated if not headSha; upgrade path is passing isCommitOutdated comparing game files + const isOutdated = isCommitOutdated ?? ((commitId) => Boolean(headSha) && commitId !== headSha); + const latestByReviewer = new Map(); for (const review of reviews) { const login = review.user?.login?.toLowerCase(); @@ -64,10 +67,10 @@ export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha } const active = [...latestByReviewer.values()]; - if (active.some((r) => r.state === "CHANGES_REQUESTED" && (!headSha || r.commit_id === headSha))) { + if (active.some((r) => r.state === "CHANGES_REQUESTED" && !isOutdated(r.commit_id))) { return "changes_requested"; } - if (active.some((r) => r.state === "APPROVED" && Boolean(headSha) && r.commit_id === headSha)) { + if (active.some((r) => r.state === "APPROVED" && Boolean(headSha) && !isOutdated(r.commit_id))) { return "approved"; } if (active.some((r) => r.state === "DISMISSED")) { diff --git a/.github/scripts/review-state.test.mjs b/.github/scripts/review-state.test.mjs index cd8fab3a1d..d3fb0c63a8 100644 --- a/.github/scripts/review-state.test.mjs +++ b/.github/scripts/review-state.test.mjs @@ -1,5 +1,6 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { autoReviewLabelChanges, reconcileReviewStatus } from "./review-state.mjs"; +import { isReviewCommitOutdated } from "./review-utils.mjs"; describe("autoReviewLabelChanges", () => { it("sets Ready for Playtest on passing validation for new submissions", () => { @@ -203,4 +204,122 @@ describe("reconcileReviewStatus", () => { it("returns unknown when reviewers set is empty or undefined", () => { expect(reconcileReviewStatus({ reviews: [], reviewers: null, authorLogin: "author1", headSha })).toBe("unknown"); }); + + it("preserves changes_requested across non-game commits when isCommitOutdated is false", () => { + const reviews = [ + { user: { login: "SSoggyTacoMan" }, state: "CHANGES_REQUESTED", commit_id: "mergeSha", submitted_at: "2026-10-04T09:00:00Z" }, + ]; + const status = reconcileReviewStatus({ + reviews, + reviewers, + authorLogin: "author1", + headSha, + isCommitOutdated: (cid) => cid !== "mergeSha", + }); + expect(status).toBe("changes_requested"); + }); + + it("preserves approved across non-game commits when isCommitOutdated is false", () => { + const reviews = [ + { user: { login: "LucasHT22" }, state: "APPROVED", commit_id: "mergeSha", submitted_at: "2026-10-04T09:00:00Z" }, + ]; + const status = reconcileReviewStatus({ + reviews, + reviewers, + authorLogin: "author1", + headSha, + isCommitOutdated: (cid) => cid !== "mergeSha", + }); + expect(status).toBe("approved"); + }); + + it("resets changes_requested to none when isCommitOutdated is true", () => { + const reviews = [ + { user: { login: "SSoggyTacoMan" }, state: "CHANGES_REQUESTED", commit_id: "olderSha", submitted_at: "2026-10-04T09:00:00Z" }, + ]; + const status = reconcileReviewStatus({ + reviews, + reviewers, + authorLogin: "author1", + headSha, + isCommitOutdated: () => true, + }); + expect(status).toBe("none"); + }); +}); + +describe("isReviewCommitOutdated", () => { + it("returns false when commitId matches headSha or either is missing", async () => { + expect(await isReviewCommitOutdated({ commitId: "abc", headSha: "abc" })).toBe(false); + expect(await isReviewCommitOutdated({ commitId: null, headSha: "abc" })).toBe(false); + expect(await isReviewCommitOutdated({ commitId: "abc", headSha: null })).toBe(false); + }); + + it("returns false when compare diff does not touch submission game files", async () => { + const originalFetch = globalThis.fetch; + globalThis.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + text: async () => JSON.stringify({ files: [{ filename: "README.md" }, { filename: "games/other.js" }] }), + }); + + try { + const outdated = await isReviewCommitOutdated({ + owner: "hackclub", + repo: "sprig", + token: "token", + prNumber: 123, + commitId: "sha1", + headSha: "sha2", + submissionFiles: [{ filename: "games/my-game.js" }], + }); + expect(outdated).toBe(false); + } finally { + globalThis.fetch = originalFetch; + } + }); + + it("returns true when compare diff touches submission game files", async () => { + const originalFetch = globalThis.fetch; + globalThis.fetch = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + text: async () => JSON.stringify({ files: [{ filename: "games/my-game.js" }] }), + }); + + try { + const outdated = await isReviewCommitOutdated({ + owner: "hackclub", + repo: "sprig", + token: "token", + prNumber: 123, + commitId: "sha1", + headSha: "sha2", + submissionFiles: [{ filename: "games/my-game.js" }], + }); + expect(outdated).toBe(true); + } finally { + globalThis.fetch = originalFetch; + } + }); + + it("returns true as safe fallback when compare request throws", async () => { + const originalFetch = globalThis.fetch; + globalThis.fetch = vi.fn().mockRejectedValue(new Error("Network error")); + + try { + const outdated = await isReviewCommitOutdated({ + owner: "hackclub", + repo: "sprig", + token: "token", + prNumber: 123, + commitId: "sha1", + headSha: "sha2", + submissionFiles: [{ filename: "games/my-game.js" }], + }); + expect(outdated).toBe(true); + } finally { + globalThis.fetch = originalFetch; + } + }); }); diff --git a/.github/scripts/review-utils.mjs b/.github/scripts/review-utils.mjs index 4262bff6fb..de402b60e3 100644 --- a/.github/scripts/review-utils.mjs +++ b/.github/scripts/review-utils.mjs @@ -276,6 +276,29 @@ export function daysBetween(start, end = new Date()) { return Math.floor((endTime - startTime) / 86_400_000); } +export async function isReviewCommitOutdated({ owner, repo, token, prNumber, commitId, headSha, submissionFiles }) { + if (!commitId || !headSha || commitId === headSha) return false; + try { + const files = submissionFiles ?? (await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/files`)); + const submissionFilenames = new Set( + files + .map((f) => (typeof f === "string" ? f : f.filename)) + .filter((fn) => fn.toLowerCase().startsWith("games/")) + ); + if (submissionFilenames.size === 0) return true; + + const compare = await githubRequest( + token, + "GET", + `/repos/${owner}/${repo}/compare/${encodeURIComponent(commitId)}...${encodeURIComponent(headSha)}` + ); + const changedFiles = compare.files ?? []; + return changedFiles.some((f) => submissionFilenames.has(f.filename)); + } catch { + return true; + } +} + export function getMetadataValue(content, key) { const match = content.match(new RegExp(String.raw`@${key}:[^\S\r\n]*([\s\S]*?)(?=\r?\n\s*\*?\s*@|\r?\n\s*\*\/|\*\/)`, "i")); if (!match?.[1]) return ""; diff --git a/.github/scripts/stale-review-queue.mjs b/.github/scripts/stale-review-queue.mjs index 568210b365..49617ff659 100644 --- a/.github/scripts/stale-review-queue.mjs +++ b/.github/scripts/stale-review-queue.mjs @@ -140,7 +140,7 @@ async function handleNeedsAuthor(pullRequest, labels) { if (!since) return; const authorLogin = pullRequest.user?.login; - const lastAuthorActivity = await latestAuthorCommentTime(pullRequest.number, authorLogin); + const lastAuthorActivity = await latestAuthorActivityTime(pullRequest.number, authorLogin); if (lastAuthorActivity && new Date(lastAuthorActivity).getTime() > new Date(since).getTime()) { since = lastAuthorActivity; } @@ -211,7 +211,7 @@ async function handleClaimed(pullRequest) { async function handleUntouchedSubmission(pullRequest, labels) { if (hasLabel(labels, "Stale")) { const authorLogin = pullRequest.user?.login; - const lastAuthorActivity = await latestAuthorCommentTime(pullRequest.number, authorLogin); + const lastAuthorActivity = await latestAuthorActivityTime(pullRequest.number, authorLogin); const staleTime = await latestLabelTime(pullRequest.number, "Stale"); if (lastAuthorActivity && staleTime && new Date(lastAuthorActivity).getTime() > new Date(staleTime).getTime()) { await removeLabel({ owner, repo, token, issueNumber: pullRequest.number, label: "Stale" }); @@ -222,7 +222,8 @@ async function handleUntouchedSubmission(pullRequest, labels) { return; } - if (daysBetween(pullRequest.updated_at) >= 30) { + const lastActivity = await latestSubmissionActivityTime(pullRequest); + if (daysBetween(lastActivity) >= 30) { await addLabels({ owner, repo, token, issueNumber: pullRequest.number, labels: ["Stale"] }); await commentOnce({ issueNumber: pullRequest.number, @@ -240,19 +241,49 @@ async function latestLabelTime(issueNumber, labelName) { return matching[0]?.created_at ?? null; } -async function latestAuthorCommentTime(issueNumber, authorLogin) { +async function latestAuthorActivityTime(issueNumber, authorLogin) { if (!authorLogin) return null; + const lowerAuthor = authorLogin.toLowerCase(); try { - const comments = await githubPaginated(token, `/repos/${owner}/${repo}/issues/${issueNumber}/comments`); + const [comments, commits] = await Promise.all([ + githubPaginated(token, `/repos/${owner}/${repo}/issues/${issueNumber}/comments`), + githubPaginated(token, `/repos/${owner}/${repo}/pulls/${issueNumber}/commits`), + ]); const authorComments = comments - .filter((c) => c.user?.login?.toLowerCase() === authorLogin.toLowerCase()) - .sort((a, b) => new Date(b.created_at).getTime() - new Date(a.created_at).getTime()); - return authorComments[0]?.created_at ?? null; + .filter((c) => c.user?.login?.toLowerCase() === lowerAuthor) + .map((c) => c.created_at); + const authorCommits = commits + .filter((c) => c.author?.login?.toLowerCase() === lowerAuthor || c.committer?.login?.toLowerCase() === lowerAuthor) + .map((c) => c.commit?.author?.date || c.commit?.committer?.date) + .filter(Boolean); + return newestDate([...authorComments, ...authorCommits]); } catch { return null; } } +async function latestSubmissionActivityTime(pullRequest) { + const fallback = pullRequest.created_at; + const authorLogin = pullRequest.user?.login; + const lowerAuthor = authorLogin?.toLowerCase(); + try { + const [comments, commits] = await Promise.all([ + githubPaginated(token, `/repos/${owner}/${repo}/issues/${pullRequest.number}/comments`), + githubPaginated(token, `/repos/${owner}/${repo}/pulls/${pullRequest.number}/commits`), + ]); + const humanComments = comments + .filter((c) => c.user?.type !== "Bot") + .map((c) => c.created_at); + const authorCommits = commits + .filter((c) => !lowerAuthor || c.author?.login?.toLowerCase() === lowerAuthor) + .map((c) => c.commit?.author?.date) + .filter(Boolean); + return newestDate([...humanComments, ...authorCommits, fallback]); + } catch { + return pullRequest.updated_at ?? fallback; + } +} + async function commentOnce({ issueNumber, marker, body }) { const comments = await githubPaginated(token, `/repos/${owner}/${repo}/issues/${issueNumber}/comments`); const alreadyCommented = comments.some((comment) => comment.body?.includes(marker)); diff --git a/.github/scripts/sync-reviews.mjs b/.github/scripts/sync-reviews.mjs index 49da223c1a..972140ce84 100644 --- a/.github/scripts/sync-reviews.mjs +++ b/.github/scripts/sync-reviews.mjs @@ -8,6 +8,7 @@ import { githubPaginated, githubRequest, hasLabel, + isReviewCommitOutdated, readGitHubEvent, setStateLabel, } from "./review-utils.mjs"; @@ -74,11 +75,26 @@ async function syncSinglePR(prNumber) { if (!hasLabel(labels, "Submission")) return; const reviews = await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/reviews`); + const outdatedCommitIds = new Set(); + for (const r of reviews) { + if (r.commit_id && r.commit_id !== pull.head?.sha) { + const isOutdated = await isReviewCommitOutdated({ + owner, + repo, + token, + prNumber, + commitId: r.commit_id, + headSha: pull.head?.sha, + }); + if (isOutdated) outdatedCommitIds.add(r.commit_id); + } + } const status = reconcileReviewStatus({ reviews, reviewers, authorLogin: pull.user?.login, headSha: pull.head?.sha, + isCommitOutdated: (cid) => cid !== pull.head?.sha && outdatedCommitIds.has(cid), }); await applyReviewStatus({ issueNumber: prNumber, status }); @@ -154,11 +170,27 @@ async function syncAllOpenSubmissions() { submitted_at: r.submittedAt, })); + const outdatedCommitIds = new Set(); + for (const r of reviews) { + if (r.commit_id && r.commit_id !== node.headRefOid) { + const isOutdated = await isReviewCommitOutdated({ + owner, + repo, + token, + prNumber: issueNumber, + commitId: r.commit_id, + headSha: node.headRefOid, + }); + if (isOutdated) outdatedCommitIds.add(r.commit_id); + } + } + const status = reconcileReviewStatus({ reviews, reviewers, authorLogin: node.author?.login, headSha: node.headRefOid, + isCommitOutdated: (cid) => cid !== node.headRefOid && outdatedCommitIds.has(cid), }); try { From 31feeb7f7fff0046d8b138aa454b41e2e4ce7470 Mon Sep 17 00:00:00 2001 From: Maaren <80040706+SSoggyTacoMan@users.noreply.github.com> Date: Sat, 17 Oct 2026 03:32:08 +0200 Subject: [PATCH 2/3] fix(ci): optimize review reconciliation queries and harden commit checking --- .github/scripts/auto-triage.mjs | 36 +++++++------ .github/scripts/review-state.mjs | 19 ++++--- .github/scripts/review-state.test.mjs | 31 ++++++++++- .github/scripts/stale-review-queue.mjs | 15 +++++- .github/scripts/sync-reviews.mjs | 74 +++++++++++++++----------- 5 files changed, 120 insertions(+), 55 deletions(-) diff --git a/.github/scripts/auto-triage.mjs b/.github/scripts/auto-triage.mjs index 234aca7044..8b97dde46b 100644 --- a/.github/scripts/auto-triage.mjs +++ b/.github/scripts/auto-triage.mjs @@ -33,7 +33,7 @@ import { findDuplicateGroup, parseOlderNotice, } from "./duplicate-detection.mjs"; -import { autoReviewLabelChanges, reconcileReviewStatus } from "./review-state.mjs"; +import { autoReviewLabelChanges, getActiveReviews, reconcileReviewStatus } from "./review-state.mjs"; const token = process.env.GITHUB_TOKEN; if (!token) throw new Error("GITHUB_TOKEN is required"); @@ -528,27 +528,33 @@ async function getLatestReviewStatus({ owner, repo, token, prNumber, reviewers, if (!reviewers || reviewers.size === 0) return "unknown"; try { const reviews = await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/reviews`); + const active = getActiveReviews({ reviews, reviewers, authorLogin }); + const pendingCommits = [ + ...new Set( + active + .filter((r) => (r.state === "CHANGES_REQUESTED" || r.state === "APPROVED") && r.commit_id && r.commit_id !== headSha) + .map((r) => r.commit_id) + ), + ]; const outdatedCommitIds = new Set(); - for (const r of reviews) { - if (r.commit_id && r.commit_id !== headSha) { - const isOutdated = await isReviewCommitOutdated({ - owner, - repo, - token, - prNumber, - commitId: r.commit_id, - headSha, - submissionFiles: pullFiles, - }); - if (isOutdated) outdatedCommitIds.add(r.commit_id); - } + for (const commitId of pendingCommits) { + const isOutdated = await isReviewCommitOutdated({ + owner, + repo, + token, + prNumber, + commitId, + headSha, + submissionFiles: pullFiles, + }); + if (isOutdated) outdatedCommitIds.add(commitId); } return reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha, - isCommitOutdated: (cid) => cid !== headSha && outdatedCommitIds.has(cid), + isCommitOutdated: (cid) => !cid || (cid !== headSha && outdatedCommitIds.has(cid)), }); } catch (e) { console.warn("Unable to fetch reviews; skipping review reconciliation:", e.message); diff --git a/.github/scripts/review-state.mjs b/.github/scripts/review-state.mjs index 43c4daeac8..c9be086263 100644 --- a/.github/scripts/review-state.mjs +++ b/.github/scripts/review-state.mjs @@ -47,14 +47,11 @@ export function autoReviewLabelChanges({ labels, validationOk, eventAction, revi } } -export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha, isCommitOutdated }) { - if (!reviewers || reviewers.size === 0) return "unknown"; +export function getActiveReviews({ reviews, reviewers, authorLogin }) { + if (!reviewers || reviewers.size === 0) return []; const lowerReviewers = new Set([...reviewers].map((u) => u.toLowerCase())); const lowerAuthor = authorLogin?.toLowerCase(); - // ponytail: default considers a commit outdated if not headSha; upgrade path is passing isCommitOutdated comparing game files - const isOutdated = isCommitOutdated ?? ((commitId) => Boolean(headSha) && commitId !== headSha); - const latestByReviewer = new Map(); for (const review of reviews) { const login = review.user?.login?.toLowerCase(); @@ -66,7 +63,17 @@ export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha } } - const active = [...latestByReviewer.values()]; + return [...latestByReviewer.values()]; +} + +export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha, isCommitOutdated }) { + if (!reviewers || reviewers.size === 0) return "unknown"; + + // ponytail: missing commit ID or mismatch defaults to outdated; upgrade path is passing isCommitOutdated comparing game files + const isOutdated = (commitId) => + !commitId || (isCommitOutdated ? isCommitOutdated(commitId) : Boolean(headSha) && commitId !== headSha); + + const active = getActiveReviews({ reviews, reviewers, authorLogin }); if (active.some((r) => r.state === "CHANGES_REQUESTED" && !isOutdated(r.commit_id))) { return "changes_requested"; } diff --git a/.github/scripts/review-state.test.mjs b/.github/scripts/review-state.test.mjs index d3fb0c63a8..345f1ed7c4 100644 --- a/.github/scripts/review-state.test.mjs +++ b/.github/scripts/review-state.test.mjs @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import { autoReviewLabelChanges, reconcileReviewStatus } from "./review-state.mjs"; +import { autoReviewLabelChanges, getActiveReviews, reconcileReviewStatus } from "./review-state.mjs"; import { isReviewCommitOutdated } from "./review-utils.mjs"; describe("autoReviewLabelChanges", () => { @@ -246,6 +246,35 @@ describe("reconcileReviewStatus", () => { }); expect(status).toBe("none"); }); + + it("treats null or missing commit_id as outdated even with custom isCommitOutdated", () => { + const reviews = [ + { user: { login: "LucasHT22" }, state: "APPROVED", commit_id: null, submitted_at: "2026-10-04T09:00:00Z" }, + ]; + const status = reconcileReviewStatus({ + reviews, + reviewers, + authorLogin: "author1", + headSha, + isCommitOutdated: () => false, + }); + expect(status).toBe("none"); + }); +}); + +describe("getActiveReviews", () => { + it("filters out non-reviewers, author, and non-actionable review states", () => { + const reviews = [ + { user: { login: "random" }, state: "APPROVED", commit_id: "sha1", submitted_at: "2026-10-04T09:00:00Z" }, + { user: { login: "author1" }, state: "APPROVED", commit_id: "sha1", submitted_at: "2026-10-04T09:00:00Z" }, + { user: { login: "SSoggyTacoMan" }, state: "COMMENTED", commit_id: "sha1", submitted_at: "2026-10-04T09:00:00Z" }, + { user: { login: "SSoggyTacoMan" }, state: "CHANGES_REQUESTED", commit_id: "sha2", submitted_at: "2026-10-04T10:00:00Z" }, + { user: { login: "LucasHT22" }, state: "APPROVED", commit_id: "sha3", submitted_at: "2026-10-04T11:00:00Z" }, + ]; + const active = getActiveReviews({ reviews, reviewers: new Set(["ssoggytacoman", "lucasht22"]), authorLogin: "author1" }); + expect(active).toHaveLength(2); + expect(active.map((r) => r.user.login)).toEqual(["SSoggyTacoMan", "LucasHT22"]); + }); }); describe("isReviewCommitOutdated", () => { diff --git a/.github/scripts/stale-review-queue.mjs b/.github/scripts/stale-review-queue.mjs index 49617ff659..1653d0db59 100644 --- a/.github/scripts/stale-review-queue.mjs +++ b/.github/scripts/stale-review-queue.mjs @@ -175,6 +175,8 @@ async function handleNeedsAuthor(pullRequest, labels) { function newestDate(dates) { return dates + .filter(Boolean) + .filter((d) => !Number.isNaN(new Date(d).getTime())) .sort((a, b) => new Date(b).getTime() - new Date(a).getTime())[0] ?? null; } @@ -222,6 +224,10 @@ async function handleUntouchedSubmission(pullRequest, labels) { return; } + if (daysBetween(pullRequest.created_at) < 30) { + return; + } + const lastActivity = await latestSubmissionActivityTime(pullRequest); if (daysBetween(lastActivity) >= 30) { await addLabels({ owner, repo, token, issueNumber: pullRequest.number, labels: ["Stale"] }); @@ -275,8 +281,13 @@ async function latestSubmissionActivityTime(pullRequest) { .filter((c) => c.user?.type !== "Bot") .map((c) => c.created_at); const authorCommits = commits - .filter((c) => !lowerAuthor || c.author?.login?.toLowerCase() === lowerAuthor) - .map((c) => c.commit?.author?.date) + .filter( + (c) => + !lowerAuthor || + c.author?.login?.toLowerCase() === lowerAuthor || + c.committer?.login?.toLowerCase() === lowerAuthor + ) + .map((c) => c.commit?.author?.date || c.commit?.committer?.date) .filter(Boolean); return newestDate([...humanComments, ...authorCommits, fallback]); } catch { diff --git a/.github/scripts/sync-reviews.mjs b/.github/scripts/sync-reviews.mjs index 972140ce84..0dbc8f6b81 100644 --- a/.github/scripts/sync-reviews.mjs +++ b/.github/scripts/sync-reviews.mjs @@ -12,7 +12,7 @@ import { readGitHubEvent, setStateLabel, } from "./review-utils.mjs"; -import { reconcileReviewStatus } from "./review-state.mjs"; +import { getActiveReviews, reconcileReviewStatus } from "./review-state.mjs"; const token = process.env.GITHUB_TOKEN; if (!token) throw new Error("GITHUB_TOKEN is required"); @@ -68,6 +68,34 @@ if (dispatchedPr && !Number.isNaN(dispatchedPr)) { await syncAllOpenSubmissions(); +async function resolveOutdatedCommitIds({ prNumber, reviews, authorLogin, headSha }) { + const active = getActiveReviews({ reviews, reviewers, authorLogin }); + const pendingCommits = [ + ...new Set( + active + .filter((r) => (r.state === "CHANGES_REQUESTED" || r.state === "APPROVED") && r.commit_id && r.commit_id !== headSha) + .map((r) => r.commit_id) + ), + ]; + const outdatedCommitIds = new Set(); + if (pendingCommits.length === 0) return outdatedCommitIds; + + const submissionFiles = await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/files`); + for (const commitId of pendingCommits) { + const isOutdated = await isReviewCommitOutdated({ + owner, + repo, + token, + prNumber, + commitId, + headSha, + submissionFiles, + }); + if (isOutdated) outdatedCommitIds.add(commitId); + } + return outdatedCommitIds; +} + async function syncSinglePR(prNumber) { const pull = await githubRequest(token, "GET", `/repos/${owner}/${repo}/pulls/${prNumber}`); if (pull.state !== "open" || pull.draft) return; @@ -75,26 +103,18 @@ async function syncSinglePR(prNumber) { if (!hasLabel(labels, "Submission")) return; const reviews = await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/reviews`); - const outdatedCommitIds = new Set(); - for (const r of reviews) { - if (r.commit_id && r.commit_id !== pull.head?.sha) { - const isOutdated = await isReviewCommitOutdated({ - owner, - repo, - token, - prNumber, - commitId: r.commit_id, - headSha: pull.head?.sha, - }); - if (isOutdated) outdatedCommitIds.add(r.commit_id); - } - } + const outdatedCommitIds = await resolveOutdatedCommitIds({ + prNumber, + reviews, + authorLogin: pull.user?.login, + headSha: pull.head?.sha, + }); const status = reconcileReviewStatus({ reviews, reviewers, authorLogin: pull.user?.login, headSha: pull.head?.sha, - isCommitOutdated: (cid) => cid !== pull.head?.sha && outdatedCommitIds.has(cid), + isCommitOutdated: (cid) => !cid || (cid !== pull.head?.sha && outdatedCommitIds.has(cid)), }); await applyReviewStatus({ issueNumber: prNumber, status }); @@ -170,27 +190,19 @@ async function syncAllOpenSubmissions() { submitted_at: r.submittedAt, })); - const outdatedCommitIds = new Set(); - for (const r of reviews) { - if (r.commit_id && r.commit_id !== node.headRefOid) { - const isOutdated = await isReviewCommitOutdated({ - owner, - repo, - token, - prNumber: issueNumber, - commitId: r.commit_id, - headSha: node.headRefOid, - }); - if (isOutdated) outdatedCommitIds.add(r.commit_id); - } - } + const outdatedCommitIds = await resolveOutdatedCommitIds({ + prNumber: issueNumber, + reviews, + authorLogin: node.author?.login, + headSha: node.headRefOid, + }); const status = reconcileReviewStatus({ reviews, reviewers, authorLogin: node.author?.login, headSha: node.headRefOid, - isCommitOutdated: (cid) => cid !== node.headRefOid && outdatedCommitIds.has(cid), + isCommitOutdated: (cid) => !cid || (cid !== node.headRefOid && outdatedCommitIds.has(cid)), }); try { From 2786fe8435049d95e19d34471e028e2b64d1378c Mon Sep 17 00:00:00 2001 From: Maaren <80040706+SSoggyTacoMan@users.noreply.github.com> Date: Sat, 17 Oct 2026 05:17:26 +0200 Subject: [PATCH 3/3] fix(ci): optimize review commit diffs with blob sha checks and harden author activity detection --- .github/scripts/auto-triage.mjs | 11 ++----- .github/scripts/review-state.mjs | 11 +++++++ .github/scripts/review-state.test.mjs | 28 +++++++++++------ .github/scripts/review-utils.mjs | 42 +++++++++++++++++++------- .github/scripts/stale-review-queue.mjs | 26 +++++++++++----- .github/scripts/sync-reviews.mjs | 11 ++----- 6 files changed, 83 insertions(+), 46 deletions(-) diff --git a/.github/scripts/auto-triage.mjs b/.github/scripts/auto-triage.mjs index 8b97dde46b..aa36161aa5 100644 --- a/.github/scripts/auto-triage.mjs +++ b/.github/scripts/auto-triage.mjs @@ -33,7 +33,7 @@ import { findDuplicateGroup, parseOlderNotice, } from "./duplicate-detection.mjs"; -import { autoReviewLabelChanges, getActiveReviews, reconcileReviewStatus } from "./review-state.mjs"; +import { autoReviewLabelChanges, getActiveReviewCommits, reconcileReviewStatus } from "./review-state.mjs"; const token = process.env.GITHUB_TOKEN; if (!token) throw new Error("GITHUB_TOKEN is required"); @@ -528,14 +528,7 @@ async function getLatestReviewStatus({ owner, repo, token, prNumber, reviewers, if (!reviewers || reviewers.size === 0) return "unknown"; try { const reviews = await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/reviews`); - const active = getActiveReviews({ reviews, reviewers, authorLogin }); - const pendingCommits = [ - ...new Set( - active - .filter((r) => (r.state === "CHANGES_REQUESTED" || r.state === "APPROVED") && r.commit_id && r.commit_id !== headSha) - .map((r) => r.commit_id) - ), - ]; + const pendingCommits = getActiveReviewCommits({ reviews, reviewers, authorLogin, headSha }); const outdatedCommitIds = new Set(); for (const commitId of pendingCommits) { const isOutdated = await isReviewCommitOutdated({ diff --git a/.github/scripts/review-state.mjs b/.github/scripts/review-state.mjs index c9be086263..4aab4c8bb5 100644 --- a/.github/scripts/review-state.mjs +++ b/.github/scripts/review-state.mjs @@ -66,6 +66,17 @@ export function getActiveReviews({ reviews, reviewers, authorLogin }) { return [...latestByReviewer.values()]; } +export function getActiveReviewCommits({ reviews, reviewers, authorLogin, headSha }) { + const active = getActiveReviews({ reviews, reviewers, authorLogin }); + const commits = new Set(); + for (const r of active) { + if ((r.state === "APPROVED" || r.state === "CHANGES_REQUESTED") && r.commit_id && r.commit_id !== headSha) { + commits.add(r.commit_id); + } + } + return [...commits]; +} + export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha, isCommitOutdated }) { if (!reviewers || reviewers.size === 0) return "unknown"; diff --git a/.github/scripts/review-state.test.mjs b/.github/scripts/review-state.test.mjs index 345f1ed7c4..e9113ef5db 100644 --- a/.github/scripts/review-state.test.mjs +++ b/.github/scripts/review-state.test.mjs @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import { autoReviewLabelChanges, getActiveReviews, reconcileReviewStatus } from "./review-state.mjs"; +import { autoReviewLabelChanges, getActiveReviewCommits, getActiveReviews, reconcileReviewStatus } from "./review-state.mjs"; import { isReviewCommitOutdated } from "./review-utils.mjs"; describe("autoReviewLabelChanges", () => { @@ -262,7 +262,7 @@ describe("reconcileReviewStatus", () => { }); }); -describe("getActiveReviews", () => { +describe("getActiveReviews and getActiveReviewCommits", () => { it("filters out non-reviewers, author, and non-actionable review states", () => { const reviews = [ { user: { login: "random" }, state: "APPROVED", commit_id: "sha1", submitted_at: "2026-10-04T09:00:00Z" }, @@ -274,6 +274,14 @@ describe("getActiveReviews", () => { const active = getActiveReviews({ reviews, reviewers: new Set(["ssoggytacoman", "lucasht22"]), authorLogin: "author1" }); expect(active).toHaveLength(2); expect(active.map((r) => r.user.login)).toEqual(["SSoggyTacoMan", "LucasHT22"]); + + const commits = getActiveReviewCommits({ + reviews, + reviewers: new Set(["ssoggytacoman", "lucasht22"]), + authorLogin: "author1", + headSha: "sha3", + }); + expect(commits).toEqual(["sha2"]); }); }); @@ -284,12 +292,12 @@ describe("isReviewCommitOutdated", () => { expect(await isReviewCommitOutdated({ commitId: "abc", headSha: null })).toBe(false); }); - it("returns false when compare diff does not touch submission game files", async () => { + it("returns false when file blob SHA at commitId matches headSha", async () => { const originalFetch = globalThis.fetch; globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, status: 200, - text: async () => JSON.stringify({ files: [{ filename: "README.md" }, { filename: "games/other.js" }] }), + text: async () => JSON.stringify({ sha: "blobSha1" }), }); try { @@ -300,7 +308,7 @@ describe("isReviewCommitOutdated", () => { prNumber: 123, commitId: "sha1", headSha: "sha2", - submissionFiles: [{ filename: "games/my-game.js" }], + submissionFiles: [{ filename: "games/my-game.js", sha: "blobSha1" }], }); expect(outdated).toBe(false); } finally { @@ -308,12 +316,12 @@ describe("isReviewCommitOutdated", () => { } }); - it("returns true when compare diff touches submission game files", async () => { + it("returns true when file blob SHA at commitId differs from headSha", async () => { const originalFetch = globalThis.fetch; globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, status: 200, - text: async () => JSON.stringify({ files: [{ filename: "games/my-game.js" }] }), + text: async () => JSON.stringify({ sha: "differentBlobSha" }), }); try { @@ -324,7 +332,7 @@ describe("isReviewCommitOutdated", () => { prNumber: 123, commitId: "sha1", headSha: "sha2", - submissionFiles: [{ filename: "games/my-game.js" }], + submissionFiles: [{ filename: "games/my-game.js", sha: "blobSha1" }], }); expect(outdated).toBe(true); } finally { @@ -332,7 +340,7 @@ describe("isReviewCommitOutdated", () => { } }); - it("returns true as safe fallback when compare request throws", async () => { + it("returns true as safe fallback when contents request throws", async () => { const originalFetch = globalThis.fetch; globalThis.fetch = vi.fn().mockRejectedValue(new Error("Network error")); @@ -344,7 +352,7 @@ describe("isReviewCommitOutdated", () => { prNumber: 123, commitId: "sha1", headSha: "sha2", - submissionFiles: [{ filename: "games/my-game.js" }], + submissionFiles: [{ filename: "games/my-game.js", sha: "blobSha1" }], }); expect(outdated).toBe(true); } finally { diff --git a/.github/scripts/review-utils.mjs b/.github/scripts/review-utils.mjs index de402b60e3..450ade2f34 100644 --- a/.github/scripts/review-utils.mjs +++ b/.github/scripts/review-utils.mjs @@ -280,22 +280,42 @@ export async function isReviewCommitOutdated({ owner, repo, token, prNumber, com if (!commitId || !headSha || commitId === headSha) return false; try { const files = submissionFiles ?? (await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/files`)); - const submissionFilenames = new Set( - files - .map((f) => (typeof f === "string" ? f : f.filename)) - .filter((fn) => fn.toLowerCase().startsWith("games/")) - ); - if (submissionFilenames.size === 0) return true; + const submissionFileObjects = files.filter((f) => { + const fn = typeof f === "string" ? f : f.filename; + return fn.toLowerCase().startsWith("games/"); + }); + if (submissionFileObjects.length === 0) return true; + + for (const file of submissionFileObjects) { + const filename = typeof file === "string" ? file : file.filename; + const targetHeadSha = + typeof file === "object" && file?.sha + ? file.sha + : await getFileBlobSha({ owner, repo, token, path: filename, ref: headSha }); + if (!targetHeadSha) return true; + + const commitBlobSha = await getFileBlobSha({ owner, repo, token, path: filename, ref: commitId }); + if (!commitBlobSha || commitBlobSha !== targetHeadSha) { + return true; + } + } - const compare = await githubRequest( + return false; + } catch { + return true; + } +} + +async function getFileBlobSha({ owner, repo, token, path: filePath, ref }) { + try { + const data = await githubRequest( token, "GET", - `/repos/${owner}/${repo}/compare/${encodeURIComponent(commitId)}...${encodeURIComponent(headSha)}` + `/repos/${owner}/${repo}/contents/${filePath.split("/").map(encodeURIComponent).join("/")}?ref=${encodeURIComponent(ref)}` ); - const changedFiles = compare.files ?? []; - return changedFiles.some((f) => submissionFilenames.has(f.filename)); + return data?.sha ?? null; } catch { - return true; + return null; } } diff --git a/.github/scripts/stale-review-queue.mjs b/.github/scripts/stale-review-queue.mjs index 1653d0db59..07b764849c 100644 --- a/.github/scripts/stale-review-queue.mjs +++ b/.github/scripts/stale-review-queue.mjs @@ -250,6 +250,7 @@ async function latestLabelTime(issueNumber, labelName) { async function latestAuthorActivityTime(issueNumber, authorLogin) { if (!authorLogin) return null; const lowerAuthor = authorLogin.toLowerCase(); + const maintainerLogins = new Set([...roles.maintainers, ...roles.triagers].map((u) => u.toLowerCase())); try { const [comments, commits] = await Promise.all([ githubPaginated(token, `/repos/${owner}/${repo}/issues/${issueNumber}/comments`), @@ -259,7 +260,14 @@ async function latestAuthorActivityTime(issueNumber, authorLogin) { .filter((c) => c.user?.login?.toLowerCase() === lowerAuthor) .map((c) => c.created_at); const authorCommits = commits - .filter((c) => c.author?.login?.toLowerCase() === lowerAuthor || c.committer?.login?.toLowerCase() === lowerAuthor) + .filter((c) => { + const isAuthor = c.author?.login?.toLowerCase() === lowerAuthor; + const isCommitter = c.committer?.login?.toLowerCase() === lowerAuthor; + const gitAuthorName = c.commit?.author?.name?.toLowerCase(); + const isGitAuthor = gitAuthorName && gitAuthorName === lowerAuthor; + const isUnlinkedAuthor = !c.author?.login && !maintainerLogins.has(c.committer?.login?.toLowerCase()); + return isAuthor || isCommitter || isGitAuthor || isUnlinkedAuthor; + }) .map((c) => c.commit?.author?.date || c.commit?.committer?.date) .filter(Boolean); return newestDate([...authorComments, ...authorCommits]); @@ -272,6 +280,7 @@ async function latestSubmissionActivityTime(pullRequest) { const fallback = pullRequest.created_at; const authorLogin = pullRequest.user?.login; const lowerAuthor = authorLogin?.toLowerCase(); + const maintainerLogins = new Set([...roles.maintainers, ...roles.triagers].map((u) => u.toLowerCase())); try { const [comments, commits] = await Promise.all([ githubPaginated(token, `/repos/${owner}/${repo}/issues/${pullRequest.number}/comments`), @@ -281,12 +290,15 @@ async function latestSubmissionActivityTime(pullRequest) { .filter((c) => c.user?.type !== "Bot") .map((c) => c.created_at); const authorCommits = commits - .filter( - (c) => - !lowerAuthor || - c.author?.login?.toLowerCase() === lowerAuthor || - c.committer?.login?.toLowerCase() === lowerAuthor - ) + .filter((c) => { + if (!lowerAuthor) return true; + const isAuthor = c.author?.login?.toLowerCase() === lowerAuthor; + const isCommitter = c.committer?.login?.toLowerCase() === lowerAuthor; + const gitAuthorName = c.commit?.author?.name?.toLowerCase(); + const isGitAuthor = gitAuthorName && gitAuthorName === lowerAuthor; + const isUnlinkedAuthor = !c.author?.login && !maintainerLogins.has(c.committer?.login?.toLowerCase()); + return isAuthor || isCommitter || isGitAuthor || isUnlinkedAuthor; + }) .map((c) => c.commit?.author?.date || c.commit?.committer?.date) .filter(Boolean); return newestDate([...humanComments, ...authorCommits, fallback]); diff --git a/.github/scripts/sync-reviews.mjs b/.github/scripts/sync-reviews.mjs index 0dbc8f6b81..0cf104f9cb 100644 --- a/.github/scripts/sync-reviews.mjs +++ b/.github/scripts/sync-reviews.mjs @@ -12,7 +12,7 @@ import { readGitHubEvent, setStateLabel, } from "./review-utils.mjs"; -import { getActiveReviews, reconcileReviewStatus } from "./review-state.mjs"; +import { getActiveReviewCommits, reconcileReviewStatus } from "./review-state.mjs"; const token = process.env.GITHUB_TOKEN; if (!token) throw new Error("GITHUB_TOKEN is required"); @@ -69,14 +69,7 @@ if (dispatchedPr && !Number.isNaN(dispatchedPr)) { await syncAllOpenSubmissions(); async function resolveOutdatedCommitIds({ prNumber, reviews, authorLogin, headSha }) { - const active = getActiveReviews({ reviews, reviewers, authorLogin }); - const pendingCommits = [ - ...new Set( - active - .filter((r) => (r.state === "CHANGES_REQUESTED" || r.state === "APPROVED") && r.commit_id && r.commit_id !== headSha) - .map((r) => r.commit_id) - ), - ]; + const pendingCommits = getActiveReviewCommits({ reviews, reviewers, authorLogin, headSha }); const outdatedCommitIds = new Set(); if (pendingCommits.length === 0) return outdatedCommitIds;