diff --git a/docs/review-automation.md b/docs/review-automation.md index 2046919..2d8c173 100644 --- a/docs/review-automation.md +++ b/docs/review-automation.md @@ -90,3 +90,5 @@ For a live staging check, use a test repository and a service credential limited Description or title changes invalidate the old review link even when the commit SHA is unchanged. A signed notification after those edits rotates the D1 review ID and sends a fresh email, including after a request for changes. A new commit also creates a fresh review. Unchanged reruns do not resend decided reviews; processing/error records require manual recovery. The internal `declined` state now means feedback was submitted, not that the PR was closed. The form uses a secret HMAC-signed bearer token, not cookie authentication. Explicit foreign origins are blocked; absent or `null` origins from privacy-preserving mail browsers are allowed only with the same valid, unused token. Fetch metadata alone cannot reject a valid Watch form. GET requests never decide a review. + +Approval evaluates the newest Actions job run per app, workflow, triggering event, and job name. GitHub's `filter=latest` still returns older suites after description edits, so repeated job names are resolved through their workflow run metadata. Runs from other workflows/apps/events remain independent blockers. Incomplete or inconsistent metadata fails closed. Public workflow metadata is readable with the current App installation; private deployments also need permission to read Actions runs. The watch error identifies failed/pending checks, validation failures, or merge conflicts separately. diff --git a/review-service/src/worker.js b/review-service/src/worker.js index afee04a..b0cdca2 100644 --- a/review-service/src/worker.js +++ b/review-service/src/worker.js @@ -44,24 +44,129 @@ async function current(api, row) { return pr; } -export async function checksPassed(api, head) { +export async function checkReadiness(api, head) { const [status, checks] = await Promise.all([ api(`/commits/${head}/status?per_page=100`), api(`/commits/${head}/check-runs?per_page=100&filter=latest`), ]); - return ( - status.state === "success" && - status.statuses.some( + const reasons = []; + if (checks.total_count > 100 || status.total_count > 100) + return { + passed: false, + reasons: ["Too many checks to verify here. Review the checks on GitHub."], + }; + for (const item of status.statuses) + if (item.state !== "success") + reasons.push(`Commit status “${item.context}” is ${item.state}.`); + if ( + !status.statuses.some( (item) => item.context === "submission-format" && item.state === "success", - ) && - checks.total_count <= 100 && - checks.check_runs.every( - (check) => - check.status === "completed" && - ["success", "neutral", "skipped"].includes(check.conclusion), ) + ) + reasons.push("The submission-format check has not passed yet."); + if (status.state !== "success" && !reasons.length) + reasons.push(`GitHub reports commit statuses as ${status.state}.`); + + // GitHub's filter=latest is per check suite, not per workflow across events. + // A description edit can create another suite on the same commit. Resolve + // duplicate Actions job names to workflow identities before replacing a run; + // unrelated workflows/apps with the same job name must never mask failures. + const groups = new Map(); + for (const check of checks.check_runs) { + const key = JSON.stringify([check.app?.id, check.name]); + if (!groups.has(key)) groups.set(key, []); + groups.get(key).push(check); + } + const candidates = checks.check_runs.filter( + (check) => + check.app?.id === 15368 && + check.app?.slug === "github-actions" && + groups.get(JSON.stringify([check.app.id, check.name])).length > 1, + ); + const runIds = new Map(); + for (const check of candidates) { + const match = + /^https:\/\/github\.com\/[^/]+\/[^/]+\/actions\/runs\/(\d+)\/job\/\d+$/.exec( + check.details_url ?? "", + ); + if (!match) + return { + passed: false, + reasons: [ + "Cannot identify repeated workflow checks. Open GitHub to review them.", + ], + }; + runIds.set(check.id, match[1]); + } + if (new Set(runIds.values()).size > 20) + return { + passed: false, + reasons: [ + "Too many repeated workflow runs to verify here. Review them on GitHub.", + ], + }; + const runs = new Map( + await Promise.all( + [...new Set(runIds.values())].map(async (id) => [ + id, + await api(`/actions/runs/${id}`), + ]), + ), + ); + const latest = new Map(); + const independent = checks.check_runs.filter( + (check) => !runIds.has(check.id), ); + for (const check of candidates) { + const run = runs.get(runIds.get(check.id)); + if ( + String(run.id) !== runIds.get(check.id) || + run.head_sha !== head || + run.check_suite_id !== check.check_suite?.id || + !Number.isSafeInteger(run.workflow_id) || + !Number.isSafeInteger(run.run_number) || + !Number.isSafeInteger(run.run_attempt) || + typeof run.event !== "string" + ) + return { + passed: false, + reasons: [ + "GitHub returned inconsistent workflow details. Retry later or inspect the checks on GitHub.", + ], + }; + const key = JSON.stringify([ + check.app.id, + run.workflow_id, + run.event, + check.name, + ]); + const previous = latest.get(key); + if ( + !previous || + run.run_number > previous.run.run_number || + (run.run_number === previous.run.run_number && + run.run_attempt > previous.run.run_attempt) || + (run.run_number === previous.run.run_number && + run.run_attempt === previous.run.run_attempt && + check.id > previous.check.id) + ) + latest.set(key, { check, run }); + } + for (const check of [ + ...independent, + ...[...latest.values()].map((item) => item.check), + ]) { + if (check.status !== "completed") + reasons.push( + `Check “${check.name}” is ${check.status}. Wait for it to finish, then retry.`, + ); + else if (!["success", "neutral", "skipped"].includes(check.conclusion)) + reasons.push( + `Check “${check.name}” finished with ${check.conclusion}. Open GitHub for its details.`, + ); + } + return { passed: reasons.length === 0, reasons }; } async function snapshotFor(api, review) { @@ -287,16 +392,22 @@ async function decide(request, env) { let commitMessage; if (action === "approve") { const review = await collectReview(api, row.pr_number); - if ( - !review.passed || - review.pr.head.sha !== row.head || - review.pr.base.sha !== row.base || - review.pr.mergeable !== true || - !(await checksPassed(api, row.head)) - ) + if (!review.passed) + return problem( + `Submission validation failed: ${review.errors.join("; ")}`, + ); + if (review.pr.head.sha !== row.head || review.pr.base.sha !== row.base) + return problem("The PR revision changed. Use the latest review email."); + if (review.pr.mergeable === null) + return problem( + "GitHub is still calculating mergeability. Wait a moment, then retry using this email.", + ); + if (review.pr.mergeable !== true) return problem( - "Checks, branch rules, or mergeability are not ready. Retry later or open GitHub.", + "This PR has merge conflicts. Resolve them on GitHub before approving.", ); + const readiness = await checkReadiness(api, row.head); + if (!readiness.passed) return problem(readiness.reasons.join(" ")); if ((env.MERGE_METHOD || "squash") === "squash" && env.REVIEWER_COAUTHOR) commitMessage = squashCommitMessage( review.commits, diff --git a/review-service/test/checks.test.js b/review-service/test/checks.test.js new file mode 100644 index 0000000..84f8651 --- /dev/null +++ b/review-service/test/checks.test.js @@ -0,0 +1,100 @@ +import { expect, test } from "vitest"; +import { checkReadiness } from "../src/worker.js"; + +const head = "a".repeat(40); +function check( + id, + conclusion = "success", + workflow = 10, + event = "pull_request_target", + app = 15368, +) { + return { + id, + name: "review", + status: "completed", + conclusion, + app: { id: app, slug: app === 15368 ? "github-actions" : "another-app" }, + details_url: `https://github.com/moepage/subdomain/actions/runs/${id}/job/${id}`, + check_suite: { id }, + run: { + id, + head_sha: head, + check_suite_id: id, + workflow_id: workflow, + run_number: id, + run_attempt: 1, + event, + }, + }; +} +function apiFor(checks, modifyRun = (run) => run) { + return async (path) => { + if (path.includes("/status?")) + return { + state: "success", + total_count: 1, + statuses: [{ context: "submission-format", state: "success" }], + }; + if (path.includes("/check-runs?")) + return { total_count: checks.length, check_runs: checks }; + if (path.startsWith("/actions/runs/")) + return modifyRun( + checks.find((check) => String(check.id) === path.split("/").at(-1)).run, + ); + throw Error(`Unexpected path ${path}`); + }; +} +test("a successful description-edit run supersedes the original failure in a different suite", async () => { + const checks = [check(1, "failure"), check(3), check(2)]; + expect(await checkReadiness(apiFor(checks), head)).toEqual({ + passed: true, + reasons: [], + }); +}); +test("latest failed or pending runs remain blockers", async () => { + for (const latest of [ + check(3, "failure"), + { ...check(3), status: "in_progress", conclusion: null }, + ]) { + const result = await checkReadiness(apiFor([check(1), latest]), head); + expect(result.passed).toBe(false); + expect(result.reasons.join()).toContain(latest.conclusion || latest.status); + } +}); +test("same job names in different workflows, events, or apps cannot hide failures", async () => { + for (const failed of [ + check(1, "failure", 20), + check(1, "failure", 10, "push"), + check(1, "failure", 10, "pull_request_target", 99), + ]) { + expect( + (await checkReadiness(apiFor([failed, check(2)]), head)).passed, + ).toBe(false); + } +}); +test("workflow identities must match the commit and check suite", async () => { + for (const values of [ + { head_sha: "b".repeat(40) }, + { check_suite_id: 999 }, + { id: 999 }, + ]) { + const result = await checkReadiness( + apiFor([check(1, "failure"), check(2)], (run) => ({ ...run, ...values })), + head, + ); + expect(result.passed).toBe(false); + expect(result.reasons.join()).toContain("inconsistent"); + } +}); +test("truncated check data cannot pass", async () => { + const api = apiFor([check(1)]); + const result = await checkReadiness( + async (path) => + path.includes("/check-runs?") + ? { total_count: 101, check_runs: [check(1)] } + : api(path), + head, + ); + expect(result.passed).toBe(false); +});