diff --git a/.github/scripts/auto-triage.mjs b/.github/scripts/auto-triage.mjs index a5963c2ce9..f42629935f 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, @@ -32,7 +33,7 @@ import { findDuplicateGroup, parseOlderNotice, } from "./duplicate-detection.mjs"; -import { autoReviewLabelChanges, 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"); @@ -523,11 +524,31 @@ 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 pendingCommits = getActiveReviewCommits({ reviews, reviewers, authorLogin, headSha }); + const outdatedCommitIds = new Set(); + 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 || (cid !== headSha && outdatedCommitIds.has(cid)), + }); } catch (e) { console.warn("Unable to fetch reviews; skipping review reconciliation:", e.message); return "unknown"; @@ -545,6 +566,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..4aab4c8bb5 100644 --- a/.github/scripts/review-state.mjs +++ b/.github/scripts/review-state.mjs @@ -47,8 +47,8 @@ export function autoReviewLabelChanges({ labels, validationOk, eventAction, revi } } -export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha }) { - 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(); @@ -63,11 +63,32 @@ export function reconcileReviewStatus({ reviews, reviewers, authorLogin, headSha } } - const active = [...latestByReviewer.values()]; - if (active.some((r) => r.state === "CHANGES_REQUESTED" && (!headSha || r.commit_id === headSha))) { + 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"; + + // 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"; } - 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..e9113ef5db 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 { autoReviewLabelChanges, reconcileReviewStatus } from "./review-state.mjs"; +import { describe, expect, it, vi } from "vitest"; +import { autoReviewLabelChanges, getActiveReviewCommits, getActiveReviews, 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,159 @@ 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"); + }); + + 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 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" }, + { 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"]); + + const commits = getActiveReviewCommits({ + reviews, + reviewers: new Set(["ssoggytacoman", "lucasht22"]), + authorLogin: "author1", + headSha: "sha3", + }); + expect(commits).toEqual(["sha2"]); + }); +}); + +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 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({ sha: "blobSha1" }), + }); + + try { + const outdated = await isReviewCommitOutdated({ + owner: "hackclub", + repo: "sprig", + token: "token", + prNumber: 123, + commitId: "sha1", + headSha: "sha2", + submissionFiles: [{ filename: "games/my-game.js", sha: "blobSha1" }], + }); + expect(outdated).toBe(false); + } finally { + globalThis.fetch = originalFetch; + } + }); + + 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({ sha: "differentBlobSha" }), + }); + + try { + const outdated = await isReviewCommitOutdated({ + owner: "hackclub", + repo: "sprig", + token: "token", + prNumber: 123, + commitId: "sha1", + headSha: "sha2", + submissionFiles: [{ filename: "games/my-game.js", sha: "blobSha1" }], + }); + expect(outdated).toBe(true); + } finally { + globalThis.fetch = originalFetch; + } + }); + + it("returns true as safe fallback when contents 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", sha: "blobSha1" }], + }); + expect(outdated).toBe(true); + } finally { + globalThis.fetch = originalFetch; + } + }); }); diff --git a/.github/scripts/review-utils.mjs b/.github/scripts/review-utils.mjs index 307024889f..77c4a6bcda 100644 --- a/.github/scripts/review-utils.mjs +++ b/.github/scripts/review-utils.mjs @@ -276,6 +276,49 @@ 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 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; + } + } + + return false; + } catch { + return true; + } +} + +async function getFileBlobSha({ owner, repo, token, path: filePath, ref }) { + try { + const data = await githubRequest( + token, + "GET", + `/repos/${owner}/${repo}/contents/${filePath.split("/").map(encodeURIComponent).join("/")}?ref=${encodeURIComponent(ref)}` + ); + return data?.sha ?? null; + } catch { + return null; + } +} + 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 2967e34380..79cf66a6db 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; } @@ -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; } @@ -211,7 +213,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 +224,12 @@ async function handleUntouchedSubmission(pullRequest, labels) { return; } - if (daysBetween(pullRequest.updated_at) >= 30) { + 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"] }); await commentOnce({ issueNumber: pullRequest.number, @@ -240,19 +247,66 @@ 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(); + const maintainerLogins = new Set([...roles.maintainers, ...roles.triagers].map((u) => u.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) => { + 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]); } catch { return null; } } +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`), + 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) => { + 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]); + } 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..0cf104f9cb 100644 --- a/.github/scripts/sync-reviews.mjs +++ b/.github/scripts/sync-reviews.mjs @@ -8,10 +8,11 @@ import { githubPaginated, githubRequest, hasLabel, + isReviewCommitOutdated, readGitHubEvent, setStateLabel, } from "./review-utils.mjs"; -import { 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"); @@ -67,6 +68,27 @@ if (dispatchedPr && !Number.isNaN(dispatchedPr)) { await syncAllOpenSubmissions(); +async function resolveOutdatedCommitIds({ prNumber, reviews, authorLogin, headSha }) { + const pendingCommits = getActiveReviewCommits({ reviews, reviewers, authorLogin, headSha }); + 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; @@ -74,11 +96,18 @@ async function syncSinglePR(prNumber) { if (!hasLabel(labels, "Submission")) return; const reviews = await githubPaginated(token, `/repos/${owner}/${repo}/pulls/${prNumber}/reviews`); + 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 || (cid !== pull.head?.sha && outdatedCommitIds.has(cid)), }); await applyReviewStatus({ issueNumber: prNumber, status }); @@ -154,11 +183,19 @@ async function syncAllOpenSubmissions() { submitted_at: r.submittedAt, })); + 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 || (cid !== node.headRefOid && outdatedCommitIds.has(cid)), }); try {