Allow approval after a corrected submission passes its latest review

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.

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,

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);
+});