From 723423ffe764ff0575f80cebe12dd2bbf773dfa4 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 28 Jul 2026 11:56:19 -0700 Subject: [PATCH] fix(ci): stop reviving superseded pull request workflows (#115378) Co-authored-by: Peter Steinberger --- scripts/github/pr-ci-sweeper.mjs | 118 +++++++++- test/scripts/pr-ci-sweeper.test.ts | 363 ++++++++++++++++++++++++++++- 2 files changed, 471 insertions(+), 10 deletions(-) diff --git a/scripts/github/pr-ci-sweeper.mjs b/scripts/github/pr-ci-sweeper.mjs index 4339c639565b..3316dbb95153 100644 --- a/scripts/github/pr-ci-sweeper.mjs +++ b/scripts/github/pr-ci-sweeper.mjs @@ -142,8 +142,8 @@ async function listLatestChecksForHead({ github, owner, repo, headSha }) { // Checks are the PR association GitHub's merge box and auto-merge actually // wait on. pull_request_target workflow runs can have a base-side head_sha // and an empty pull_requests array, so neither run field identifies the PR. - // filter=latest also omits cancelled checks superseded by a newer same-name - // check, leaving only cancelled work that still blocks this head. + // filter=latest is scoped to a check suite, not a workflow: cancelled checks + // from older runs may coexist with their completed replacements on this head. // Accepted tradeoff: checks are commit-scoped, so a second PR sharing this // exact head (a duplicate PR off the same automation branch) could have its // run revived under our candidate's eligibility. GitHub exposes no trigger @@ -159,12 +159,82 @@ async function listLatestChecksForHead({ github, owner, repo, headSha }) { }); } -function workflowRunIdForCheck(check) { - if (check.conclusion !== "cancelled" || check.app?.slug !== "github-actions") { +function githubActionsWorkflowRunIdForCheck(check) { + if (check.app?.slug !== "github-actions") { return undefined; } const match = check.details_url?.match(/\/actions\/runs\/(\d+)(?:\/|$)/); - return match ? Number(match[1]) : undefined; + if (!match) { + return undefined; + } + const runId = Number(match[1]); + return Number.isSafeInteger(runId) && runId > 0 ? runId : undefined; +} + +function workflowRunIdForCheck(check) { + if (check.conclusion !== "cancelled") { + return undefined; + } + return githubActionsWorkflowRunIdForCheck(check); +} + +async function resolveWorkflowSupersession({ + github, + owner, + repo, + checks, + run, + prCreatedAt, + prHeadBranch, + repoFullName, + workflowRunsById, +}) { + if (!Number.isSafeInteger(run.workflow_id) || run.workflow_id <= 0) { + return "unverifiable-workflow"; + } + + for (const check of checks) { + if (check.app?.slug !== "github-actions") { + continue; + } + const replacementRunId = githubActionsWorkflowRunIdForCheck(check); + if (replacementRunId === undefined) { + return "unverifiable-workflow"; + } + if (replacementRunId <= run.id) { + continue; + } + + let replacementRun = workflowRunsById.get(replacementRunId); + if (!replacementRun) { + // A run's owning workflow is immutable, so cache only this exact run-id + // lookup; current cancellation/attempt state is still fetched fresh. + replacementRun = github.rest.actions + .getWorkflowRun({ owner, repo, run_id: replacementRunId }) + .then(({ data }) => data); + workflowRunsById.set(replacementRunId, replacementRun); + } + const replacement = await replacementRun; + if (!Number.isSafeInteger(replacement?.workflow_id) || replacement.workflow_id <= 0) { + return "unverifiable-workflow"; + } + // Workflow identity alone is shared by dispatches, pushes, and other PRs. + // Match the candidate's trusted PR-event lineage, not head_sha: target + // workflows can report their base SHA rather than the checked PR head. + if ( + replacement.workflow_id === run.workflow_id && + REVIVABLE_EVENTS.has(replacement.event) && + replacement.event === run.event && + replacement.head_branch === prHeadBranch && + replacement.head_repository?.full_name === repoFullName && + Number.isFinite(Date.parse(replacement.created_at)) && + Date.parse(replacement.created_at) >= Date.parse(prCreatedAt) + ) { + return "superseded-workflow"; + } + } + + return null; } function isExpectedReviveSkip(error) { @@ -245,6 +315,7 @@ export async function runPrCiSweeper({ let revives = 0; const openPrs = await listRecentOpenPrs({ github, owner, repo, now }); const seenRunIds = new Set(); + const workflowRunsById = new Map(); reviveLane: for (const listed of openPrs) { if (now - Date.parse(listed.created_at) > LOOKBACK_MS) { break; @@ -278,6 +349,26 @@ export async function runPrCiSweeper({ prHeadBranch: listed.head.ref, repoFullName: `${owner}/${repo}`, }); + if (verdict.action === "revive") { + workflowRunsById.set(runId, Promise.resolve(run)); + const supersession = await resolveWorkflowSupersession({ + github, + owner, + repo, + checks, + run, + prCreatedAt: listed.created_at, + prHeadBranch: listed.head.ref, + repoFullName: `${owner}/${repo}`, + workflowRunsById, + }); + if (supersession) { + core.info( + `pr-ci-sweeper: skip cancelled run ${runId} for #${listed.number} (${supersession})`, + ); + continue; + } + } // Global suppression only once this run is actually handled: a // PR-relative rejection (branch mismatch, predates-pr) must leave the // run inspectable for the candidate that owns it. @@ -332,6 +423,23 @@ export async function runPrCiSweeper({ ); continue; } + const supersession = await resolveWorkflowSupersession({ + github, + owner, + repo, + checks: currentChecks, + run, + prCreatedAt: listed.created_at, + prHeadBranch: listed.head.ref, + repoFullName: `${owner}/${repo}`, + workflowRunsById, + }); + if (supersession) { + core.info( + `pr-ci-sweeper: skip cancelled run ${runId} for #${listed.number} (${supersession})`, + ); + continue; + } // Revive only a quiescent head: any queued/in-progress Actions check // means a replacement may be underway, and rerunning now could cancel // it via workflow concurrency. Auto-merge waits for every check anyway, diff --git a/test/scripts/pr-ci-sweeper.test.ts b/test/scripts/pr-ci-sweeper.test.ts index 2059d74f767d..7c248e2d05e8 100644 --- a/test/scripts/pr-ci-sweeper.test.ts +++ b/test/scripts/pr-ci-sweeper.test.ts @@ -278,12 +278,17 @@ describe("classifyRunForRevive", () => { }); type FakeCall = { method: string; args: Record }; -type FakeWorkflowRun = Parameters[0]["run"] & { id: number }; +type FakeWorkflowRun = Parameters[0]["run"] & { + id: number; + workflow_id: number | null; +}; type FakeCheckRun = { + id: number; + name: string; status?: string; conclusion: string | null; app: { slug: string } | null; - details_url: string | null; + details_url: string | null | undefined; }; function fakeGithub(options: { @@ -292,14 +297,16 @@ function fakeGithub(options: { string, Array<{ conclusion: string | null; event?: string; id?: number; status?: string }> >; - checksByRef?: Record; + checksByRef?: Record; workflowRunsById?: Record; + workflowRunErrorsById?: Record; pullsGetByNumber?: Record | Array>>; events?: Array>; pageSize?: number; }) { const calls: FakeCall[] = []; const pullsGetCallCounts = new Map(); + const checksListCallCounts = new Map(); const record = (method: string, args: Record) => { calls.push({ method, args }); }; @@ -340,7 +347,15 @@ function fakeGithub(options: { ); } if (endpoint.endpointName === "checks.listForRef") { - return Promise.resolve(options.checksByRef?.[args.ref as string] ?? []); + const ref = args.ref as string; + const configured = options.checksByRef?.[ref] ?? []; + if (Array.isArray(configured[0])) { + const snapshots = configured as FakeCheckRun[][]; + const callIndex = checksListCallCounts.get(ref) ?? 0; + checksListCallCounts.set(ref, callIndex + 1); + return Promise.resolve(snapshots[Math.min(callIndex, snapshots.length - 1)] ?? []); + } + return Promise.resolve(configured as FakeCheckRun[]); } if (endpoint.endpointName === "issues.listEvents") { return Promise.resolve(options.events ?? []); @@ -370,7 +385,12 @@ function fakeGithub(options: { listWorkflowRuns: { endpointName: "actions.listWorkflowRuns" }, getWorkflowRun: (args: Record) => { record("actions.getWorkflowRun", args); - return Promise.resolve({ data: options.workflowRunsById?.[args.run_id as number] }); + const runId = args.run_id as number; + const error = options.workflowRunErrorsById?.[runId]; + if (error) { + return Promise.reject(error); + } + return Promise.resolve({ data: options.workflowRunsById?.[runId] }); }, reRunWorkflow: (args: Record) => { record("actions.reRunWorkflow", args); @@ -415,6 +435,8 @@ function autoMergePr(number: number, headSha: string) { function githubActionsCheck(runId: number, overrides: Partial = {}): FakeCheckRun { return { + id: runId, + name: "proof", conclusion: "cancelled", status: "completed", app: { slug: "github-actions" }, @@ -426,6 +448,7 @@ function githubActionsCheck(runId: number, overrides: Partial = {} function cancelledRun(runId: number, overrides: Partial = {}): FakeWorkflowRun { return { id: runId, + workflow_id: 10, conclusion: "cancelled", event: "pull_request_target", run_attempt: 1, @@ -696,6 +719,336 @@ describe("runPrCiSweeper", () => { expect(calls.filter((call) => call.method === "pulls.update")).toEqual([]); }); + it.each([ + { + name: "same-name successful", + replacementName: "proof", + status: "completed", + conclusion: "success", + }, + { + name: "queued", + replacementName: "proof", + status: "queued", + conclusion: null, + }, + { + name: "in-progress", + replacementName: "proof", + status: "in_progress", + conclusion: null, + }, + { + name: "renamed", + replacementName: "renamed proof", + status: "completed", + conclusion: "success", + }, + ])( + "does not revive a cancelled run superseded by a $name workflow run", + async ({ replacementName, status, conclusion }) => { + const generated = autoMergePr(40, "4".repeat(40)); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [ + githubActionsCheck(100), + githubActionsCheck(200, { name: replacementName, status, conclusion }), + ], + }, + workflowRunsById: { + 100: cancelledRun(100), + 200: cancelledRun(200, { conclusion }), + }, + }); + + await runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + now: NOW, + }); + + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([]); + }, + ); + + it("does not let a newer run from another workflow suppress a cancelled run", async () => { + const generated = autoMergePr(41, "5".repeat(40)); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [ + githubActionsCheck(100), + githubActionsCheck(200, { conclusion: "success" }), + ], + }, + workflowRunsById: { + 100: cancelledRun(100), + 200: cancelledRun(200, { workflow_id: 20, conclusion: "success" }), + }, + }); + + await runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + now: NOW, + }); + + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([ + { + method: "actions.reRunWorkflow", + args: { owner: "openclaw", repo: "openclaw", run_id: 100 }, + }, + ]); + expect( + calls.filter((call) => call.method === "actions.getWorkflowRun" && call.args.run_id === 200), + ).toHaveLength(1); + }); + + it.each([ + { name: "manual dispatch", replacement: { event: "workflow_dispatch" } }, + { name: "push event", replacement: { event: "push" } }, + { name: "different pull-request event", replacement: { event: "pull_request" } }, + { name: "different head branch", replacement: { head_branch: "automation/another-pr" } }, + { name: "missing head branch", replacement: { head_branch: null } }, + { + name: "fork head repository", + replacement: { head_repository: { full_name: "someone-else/openclaw" } }, + }, + { name: "missing head repository", replacement: { head_repository: undefined } }, + { + name: "run predating the pull request", + replacement: { created_at: new Date(NOW - 3 * HOURS).toISOString() }, + }, + ])( + "does not let a newer $name from the same workflow suppress the PR run", + async ({ replacement }) => { + const generated = autoMergePr(48, "c".repeat(40)); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [ + githubActionsCheck(100), + githubActionsCheck(200, { conclusion: "success" }), + ], + }, + workflowRunsById: { + 100: cancelledRun(100), + 200: cancelledRun(200, { conclusion: "success", ...replacement }), + }, + }); + + await runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + now: NOW, + }); + + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([ + { + method: "actions.reRunWorkflow", + args: { owner: "openclaw", repo: "openclaw", run_id: 100 }, + }, + ]); + expect( + calls.filter( + (call) => call.method === "actions.getWorkflowRun" && call.args.run_id === 200, + ), + ).toHaveLength(1); + }, + ); + + it.each([ + { name: "successful", status: "completed", conclusion: "success" }, + { name: "queued", status: "queued", conclusion: null }, + { name: "in-progress", status: "in_progress", conclusion: null }, + ])("does not report a $name replacement as a dry-run revive", async ({ status, conclusion }) => { + const generated = autoMergePr(42, "6".repeat(40)); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [ + githubActionsCheck(100), + githubActionsCheck(200, { status, conclusion }), + ], + }, + workflowRunsById: { + 100: cancelledRun(100), + 200: cancelledRun(200, { conclusion }), + }, + }); + const { core: loggedCore, logs } = recordingCore(); + + await runPrCiSweeper({ + github: github as never, + context: context as never, + core: loggedCore as never, + dryRun: true, + now: NOW, + }); + + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([]); + expect(logs.some((line) => line.includes("would revive cancelled run 100"))).toBe(false); + }); + + it("does not revive a run superseded during pre-mutation revalidation", async () => { + const generated = autoMergePr(43, "7".repeat(40)); + const cancelled = githubActionsCheck(100); + const replacement = githubActionsCheck(200, { conclusion: "success" }); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [[cancelled], [cancelled, replacement]], + }, + workflowRunsById: { + 100: cancelledRun(100), + 200: cancelledRun(200, { conclusion: "success" }), + }, + }); + + await runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + now: NOW, + }); + + expect(calls.filter((call) => call.method === "checks.listForRef")).toHaveLength(2); + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([]); + }); + + it.each([ + { name: "cancelled run", cancelledWorkflowId: null, replacementWorkflowId: 10 }, + { name: "replacement run", cancelledWorkflowId: 10, replacementWorkflowId: null }, + ])( + "does not revive when the $name has no verifiable workflow identity", + async ({ cancelledWorkflowId, replacementWorkflowId }) => { + const generated = autoMergePr(44, "8".repeat(40)); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [ + githubActionsCheck(100), + githubActionsCheck(200, { conclusion: "success" }), + ], + }, + workflowRunsById: { + 100: cancelledRun(100, { workflow_id: cancelledWorkflowId }), + 200: cancelledRun(200, { + workflow_id: replacementWorkflowId, + conclusion: "success", + }), + }, + }); + + await runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + now: NOW, + }); + + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([]); + }, + ); + + it.each([ + { name: "missing", detailsUrl: null }, + { name: "undefined", detailsUrl: undefined }, + { name: "malformed", detailsUrl: "https://github.com/openclaw/openclaw/actions/runs/nope" }, + ])("does not revive when an Actions replacement has a $name run URL", async ({ detailsUrl }) => { + const generated = autoMergePr(46, "a".repeat(40)); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [ + githubActionsCheck(100), + githubActionsCheck(200, { conclusion: "success", details_url: detailsUrl }), + ], + }, + workflowRunsById: { 100: cancelledRun(100) }, + }); + + await runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + now: NOW, + }); + + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([]); + }); + + it("does not let a malformed non-Actions check suppress a cancelled workflow", async () => { + const generated = autoMergePr(47, "b".repeat(40)); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [ + githubActionsCheck(100), + githubActionsCheck(200, { + app: { slug: "external-ci" }, + conclusion: "success", + details_url: null, + }), + ], + }, + workflowRunsById: { 100: cancelledRun(100) }, + }); + + await runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + now: NOW, + }); + + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([ + { + method: "actions.reRunWorkflow", + args: { owner: "openclaw", repo: "openclaw", run_id: 100 }, + }, + ]); + }); + + it("fails closed when replacement workflow metadata cannot be loaded", async () => { + const generated = autoMergePr(45, "9".repeat(40)); + const { github, calls } = fakeGithub({ + prs: [generated], + runsBySha: {}, + checksByRef: { + [generated.head.sha]: [ + githubActionsCheck(100), + githubActionsCheck(200, { conclusion: "success" }), + ], + }, + workflowRunsById: { 100: cancelledRun(100) }, + workflowRunErrorsById: { 200: new Error("replacement workflow unavailable") }, + }); + + await expect( + runPrCiSweeper({ + github: github as never, + context: context as never, + core: core as never, + now: NOW, + }), + ).rejects.toThrow("replacement workflow unavailable"); + + expect(calls.filter((call) => call.method === "actions.reRunWorkflow")).toEqual([]); + }); + it("does not revive a run triggered from a different branch on a shared commit", async () => { const generated = autoMergePr(14, "f".repeat(40)); const { github, calls } = fakeGithub({