From 340bb359ce9e8ab08c660b03de05d814a0212ee6 Mon Sep 17 00:00:00 2001 From: simon Date: Mon, 13 Apr 2026 11:44:43 +0200 Subject: [PATCH] Fix maintainer-approval not blocking PRs without approval The previous change (PR #4931) switched maintainer-approval from commit statuses to check runs. However, check runs with status "in_progress" are treated by GitHub as "still running" and don't block the required status check, so all PRs could merge without approval. Fix: for approved PRs, create a success check run (green, clickable). For pending PRs, create nothing. The required status check stays as "Expected" in GitHub (yellow dot), which blocks the merge until approval is granted. Co-authored-by: Isaac --- .github/workflows/maintainer-approval.js | 33 ++++++------------- .github/workflows/maintainer-approval.test.js | 25 +++++--------- 2 files changed, 19 insertions(+), 39 deletions(-) diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index bf0d6522f87..daba81e106c 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -516,7 +516,10 @@ module.exports = async ({ github, context, core }) => { core ); - // Set commit status. Approved PRs return early (commit status is sufficient). + // Approved PRs get a success check run and return early. + // Pending PRs intentionally create NO check run or status. The required + // status check "maintainer-approval" stays as "Expected" (yellow dot) in + // the GitHub UI, which blocks the merge until approval is granted. if (result.allCovered && approverLogins.length > 0) { core.info("All ownership groups have per-path approval."); await github.rest.checks.create({ @@ -531,36 +534,20 @@ module.exports = async ({ github, context, core }) => { if (result.hasWildcardFiles) { const fileList = result.wildcardFiles.join(", "); - const msg = + core.info( `Files need maintainer review: ${fileList}. ` + - `Maintainers: ${maintainers.join(", ")}`; - core.info(msg); - await github.rest.checks.create({ - ...checkParams, - status: "in_progress", - output: { title: STATUS_CONTEXT, summary: msg }, - }); + `Maintainers: ${maintainers.join(", ")}` + ); } else if (result.uncovered && result.uncovered.length > 0) { const groupList = result.uncovered .map(({ pattern, owners }) => `${pattern} (needs: ${owners.join(", ")})`) .join("; "); - const msg = `Needs approval: ${groupList}`; core.info( - `${msg}. Alternatively, any maintainer can approve: ${maintainers.join(", ")}.` + `Needs approval: ${groupList}. ` + + `Alternatively, any maintainer can approve: ${maintainers.join(", ")}.` ); - await github.rest.checks.create({ - ...checkParams, - status: "in_progress", - output: { title: STATUS_CONTEXT, summary: msg }, - }); } else { - const msg = `Waiting for maintainer approval: ${maintainers.join(", ")}`; - core.info(msg); - await github.rest.checks.create({ - ...checkParams, - status: "in_progress", - output: { title: STATUS_CONTEXT, summary: msg }, - }); + core.info(`Waiting for maintainer approval: ${maintainers.join(", ")}`); } // Score contributors via git history diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js index b18bc48435e..482b60dc380 100644 --- a/.github/workflows/maintainer-approval.test.js +++ b/.github/workflows/maintainer-approval.test.js @@ -251,12 +251,11 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); - assert.equal(github._checkRuns.length, 1); - assert.equal(github._checkRuns[0].status, "in_progress"); - assert.ok(github._checkRuns[0].output.summary.includes("/bundle/")); + // No check run created; the required check stays as "Expected" (yellow dot). + assert.equal(github._checkRuns.length, 0); }); - it("wildcard files present -> pending, mentions maintainer", async () => { + it("wildcard files present -> pending, no check run", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "randomreviewer" } }, @@ -268,12 +267,10 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); - assert.equal(github._checkRuns.length, 1); - assert.equal(github._checkRuns[0].status, "in_progress"); - assert.ok(github._checkRuns[0].output.summary.includes("maintainer")); + assert.equal(github._checkRuns.length, 0); }); - it("no approvals at all -> pending", async () => { + it("no approvals at all -> pending, no check run", async () => { const github = makeGithub({ reviews: [], files: [{ filename: "cmd/pipelines/foo.go" }], @@ -283,8 +280,7 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); - assert.equal(github._checkRuns.length, 1); - assert.equal(github._checkRuns[0].status, "in_progress"); + assert.equal(github._checkRuns.length, 0); }); it("team member approved -> success for team-owned path", async () => { @@ -317,8 +313,7 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); - assert.equal(github._checkRuns.length, 1); - assert.equal(github._checkRuns[0].status, "in_progress"); + assert.equal(github._checkRuns.length, 0); }); it("CHANGES_REQUESTED does not count as approval", async () => { @@ -333,8 +328,7 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); - assert.equal(github._checkRuns.length, 1); - assert.equal(github._checkRuns[0].status, "in_progress"); + assert.equal(github._checkRuns.length, 0); }); it("self-approval by PR author is excluded", async () => { @@ -349,8 +343,7 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); - assert.equal(github._checkRuns.length, 1); - assert.equal(github._checkRuns[0].status, "in_progress"); + assert.equal(github._checkRuns.length, 0); }); it("no * rule in OWNERS -> setFailed", async () => {