From 3dfc258f2a81e3a5c6193d5915f1aa9814e81086 Mon Sep 17 00:00:00 2001 From: simon Date: Fri, 10 Apr 2026 10:43:00 +0200 Subject: [PATCH 1/2] Minimize comment noise in maintainer-approval workflow Previously the workflow deleted and recreated the PR comment on every push, review, and reopen. This caused notification noise for all participants. Now the workflow edits the existing comment in place (which does not trigger notifications), skips the edit entirely when the body has not changed, and stops posting approval comments since the commit status already conveys that information. Co-authored-by: Isaac --- .github/workflows/maintainer-approval.js | 76 +++++------ .github/workflows/maintainer-approval.test.js | 118 +++++++++++------- 2 files changed, 108 insertions(+), 86 deletions(-) diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index 13f4c6bfb6..1af528c601 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -258,32 +258,6 @@ async function selectRoundRobin(github, owner, repo, eligibleOwners, prAuthor) { // --- Comment builders --- -function buildApprovedComment(description) { - const lines = [ - MARKER, - `## ${description}`, - "", - `See ${OWNERS_LINK} for ownership rules.`, - ]; - return lines.join("\n") + "\n"; -} - -function buildAllGroupsApprovedComment(groups, approvedBy) { - const lines = [MARKER, "## All ownership groups approved", ""]; - for (const [pattern, { files }] of groups) { - if (pattern === "*") continue; - lines.push(`### \`${pattern}\` - approved`); - lines.push(`Files: ${files.map(f => `\`${f}\``).join(", ")}`); - const approver = approvedBy.get(pattern); - if (approver) { - lines.push(`Approved by: @${approver}`); - } - lines.push(""); - } - lines.push(`See ${OWNERS_LINK} for ownership rules.`); - return lines.join("\n") + "\n"; -} - function buildPendingPerGroupComment(groups, scores, dirScores, approvedBy, maintainers, prAuthor) { const authorLower = (prAuthor || "").toLowerCase(); const lines = [MARKER, "## Approval status: pending", ""]; @@ -392,20 +366,39 @@ function buildSingleDomainPendingComment(sortedScores, dirScores, scoredCount, e const LEGACY_MARKER = ""; -async function postComment(github, owner, repo, prNumber, comment) { +/** + * Create or edit the marker comment. Skips the edit if the body is unchanged. + * Cleans up duplicate or legacy marker comments, keeping only the first one. + */ +async function upsertComment(github, owner, repo, prNumber, newBody) { const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: prNumber, }); - const toDelete = comments.filter(c => + const markerComments = comments.filter(c => c.body && (c.body.includes(MARKER) || c.body.includes(LEGACY_MARKER)) ); - for (const c of toDelete) { - await github.rest.issues.deleteComment({ - owner, repo, comment_id: c.id, + + if (markerComments.length > 0) { + const existing = markerComments[0]; + + // Clean up duplicates (legacy or accidental), keep the first. + for (const c of markerComments.slice(1)) { + await github.rest.issues.deleteComment({ + owner, repo, comment_id: c.id, + }); + } + + // Skip if body is unchanged. + if (existing.body === newBody) return; + + await github.rest.issues.updateComment({ + owner, repo, comment_id: existing.id, body: newBody, }); + return; } + await github.rest.issues.createComment({ - owner, repo, issue_number: prNumber, body: comment, + owner, repo, issue_number: prNumber, body: newBody, }); } @@ -459,8 +452,6 @@ module.exports = async ({ github, context, core }) => { state: "success", description: `Approved by @${approver}`, }); - await postComment(github, owner, repo, prNumber, - buildApprovedComment(`Approved by @${approver}`)); return; } @@ -477,8 +468,6 @@ module.exports = async ({ github, context, core }) => { state: "success", description: "Approved (maintainer-authored PR)", }); - await postComment(github, owner, repo, prNumber, - buildApprovedComment("Approved (maintainer-authored PR)")); return; } } @@ -506,7 +495,7 @@ module.exports = async ({ github, context, core }) => { core ); - // Set commit status + // Set commit status. Approved PRs return early (commit status is sufficient). if (result.allCovered && approverLogins.length > 0) { core.info("All ownership groups have per-path approval."); await github.rest.repos.createCommitStatus({ @@ -514,7 +503,10 @@ module.exports = async ({ github, context, core }) => { state: "success", description: "All ownership groups approved", }); - } else if (result.hasWildcardFiles) { + return; + } + + if (result.hasWildcardFiles) { const fileList = result.wildcardFiles.join(", "); const msg = `Files need maintainer review: ${fileList}. ` + @@ -561,13 +553,11 @@ module.exports = async ({ github, context, core }) => { ); const sortedScores = Object.entries(scores).sort((a, b) => b[1] - a[1]); - // Build comment based on approval state and ownership groups + // Build pending comment with reviewer suggestions. let comment; const groups = result.groups; - if (result.allCovered && approverLogins.length > 0) { - comment = buildAllGroupsApprovedComment(groups, result.approvedBy); - } else if (groups.size >= 2) { + if (groups.size >= 2) { comment = buildPendingPerGroupComment( groups, scores, dirScores, result.approvedBy, maintainers, authorLogin ); @@ -583,5 +573,5 @@ module.exports = async ({ github, context, core }) => { } core.info(comment); - await postComment(github, owner, repo, prNumber, comment); + await upsertComment(github, owner, repo, prNumber, comment); }; diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js index 3c38ddc10d..4ea70e7fc3 100644 --- a/.github/workflows/maintainer-approval.test.js +++ b/.github/workflows/maintainer-approval.test.js @@ -62,6 +62,7 @@ function makeGithub({ reviews = [], files = [], teamMembers = {}, existingCommen const listComments = Symbol("listComments"); const statuses = []; const createdComments = []; + const updatedComments = []; const deletedCommentIds = []; const github = { @@ -89,6 +90,9 @@ function makeGithub({ reviews = [], files = [], teamMembers = {}, existingCommen createComment: async (params) => { createdComments.push(params); }, + updateComment: async (params) => { + updatedComments.push(params); + }, }, teams: { getMembershipForUserInOrg: async ({ team_slug, username }) => { @@ -103,6 +107,7 @@ function makeGithub({ reviews = [], files = [], teamMembers = {}, existingCommen }, _statuses: statuses, _comments: createdComments, + _updatedComments: updatedComments, _deletedCommentIds: deletedCommentIds, }; return github; @@ -129,7 +134,7 @@ describe("maintainer-approval", () => { fs.rmSync(tmpDir, { recursive: true }); }); - it("maintainer approved -> success", async () => { + it("maintainer approved -> success, no comment", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "maintainer1" } }, @@ -144,9 +149,11 @@ describe("maintainer-approval", () => { assert.equal(github._statuses.length, 1); assert.equal(github._statuses[0].state, "success"); assert.ok(github._statuses[0].description.includes("maintainer1")); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); }); - it("maintainer-authored PR with any approval -> success", async () => { + it("maintainer-authored PR with any approval -> success, no comment", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "randomreviewer" } }, @@ -161,9 +168,11 @@ describe("maintainer-approval", () => { assert.equal(github._statuses.length, 1); assert.equal(github._statuses[0].state, "success"); assert.ok(github._statuses[0].description.includes("maintainer-authored")); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); }); - it("single domain, owner approved -> success", async () => { + it("single domain, owner approved -> success, no comment", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "jefferycheng1" } }, @@ -180,9 +189,11 @@ describe("maintainer-approval", () => { assert.equal(github._statuses.length, 1); assert.equal(github._statuses[0].state, "success"); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); }); - it("cross-domain, both approved -> success", async () => { + it("cross-domain, both approved -> success, no comment", async () => { const github = makeGithub({ reviews: [ { state: "APPROVED", user: { login: "jefferycheng1" } }, @@ -200,6 +211,8 @@ describe("maintainer-approval", () => { assert.equal(github._statuses.length, 1); assert.equal(github._statuses[0].state, "success"); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); }); it("cross-domain, one missing -> pending", async () => { @@ -337,9 +350,9 @@ describe("maintainer-approval", () => { fs.rmSync(noWildcardDir, { recursive: true }); }); - // --- Comment posting tests --- + // --- Comment upsert tests --- - it("posts a comment with MARKER on every run", async () => { + it("creates comment with MARKER when none exists", async () => { const github = makeGithub({ reviews: [], files: [{ filename: "cmd/pipelines/foo.go" }], @@ -351,31 +364,71 @@ describe("maintainer-approval", () => { assert.equal(github._comments.length, 1); assert.ok(github._comments[0].body.includes("")); + assert.equal(github._updatedComments.length, 0); + assert.equal(github._deletedCommentIds.length, 0); }); - it("maintainer approval posts simple approved comment", async () => { + it("edits existing comment in place when body changes", async () => { const github = makeGithub({ - reviews: [ - { state: "APPROVED", user: { login: "maintainer1" } }, - ], + reviews: [], files: [{ filename: "cmd/pipelines/foo.go" }], + existingComments: [ + { id: 999, body: "\nOld comment" }, + ], }); const core = makeCore(); const context = makeContext(); await runModule({ github, context, core }); - assert.equal(github._comments.length, 1); - assert.ok(github._comments[0].body.includes("Approved by @maintainer1")); - assert.ok(github._comments[0].body.includes("")); + assert.equal(github._updatedComments.length, 1); + assert.equal(github._updatedComments[0].comment_id, 999); + assert.ok(github._updatedComments[0].body.includes("")); + assert.equal(github._comments.length, 0); + assert.equal(github._deletedCommentIds.length, 0); + }); + + it("skips edit when comment body is unchanged", async () => { + // Stub Math.random so selectRoundRobin is deterministic across runs. + const origRandom = Math.random; + Math.random = () => 0.5; + try { + // First, run once to capture the comment body. + const github1 = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core1 = makeCore(); + await runModule({ github: github1, context: makeContext(), core: core1 }); + const expectedBody = github1._comments[0].body; + + // Second run with that body as the existing comment. + const github2 = makeGithub({ + reviews: [], + files: [{ filename: "cmd/pipelines/foo.go" }], + existingComments: [ + { id: 100, body: expectedBody }, + ], + }); + const core2 = makeCore(); + await runModule({ github: github2, context: makeContext(), core: core2 }); + + assert.equal(github2._comments.length, 0); + assert.equal(github2._updatedComments.length, 0); + assert.equal(github2._deletedCommentIds.length, 0); + } finally { + Math.random = origRandom; + } }); - it("deletes existing comment before posting new one", async () => { + it("cleans up duplicate marker comments, keeps the first", async () => { const github = makeGithub({ reviews: [], files: [{ filename: "cmd/pipelines/foo.go" }], existingComments: [ - { id: 999, body: "\nOld comment" }, + { id: 100, body: "\nFirst" }, + { id: 200, body: "\nDuplicate" }, + { id: 300, body: "\nLegacy" }, ], }); const core = makeCore(); @@ -383,13 +436,14 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); - assert.equal(github._deletedCommentIds.length, 1); - assert.equal(github._deletedCommentIds[0], 999); - assert.equal(github._comments.length, 1); - assert.ok(github._comments[0].body.includes("")); + // Duplicates deleted, first one edited. + assert.deepEqual(github._deletedCommentIds.sort(), [200, 300]); + assert.equal(github._updatedComments.length, 1); + assert.equal(github._updatedComments[0].comment_id, 100); + assert.equal(github._comments.length, 0); }); - it("does not delete comments without the marker", async () => { + it("does not touch comments without the marker", async () => { const github = makeGithub({ reviews: [], files: [{ filename: "cmd/pipelines/foo.go" }], @@ -403,6 +457,7 @@ describe("maintainer-approval", () => { await runModule({ github, context, core }); assert.equal(github._deletedCommentIds.length, 0); + assert.equal(github._updatedComments.length, 0); assert.equal(github._comments.length, 1); }); @@ -443,27 +498,4 @@ describe("maintainer-approval", () => { assert.ok(body.includes("approved by @jefferycheng1")); assert.ok(body.includes("needs approval")); }); - - it("all groups approved comment shows per-group detail", async () => { - const github = makeGithub({ - reviews: [ - { state: "APPROVED", user: { login: "jefferycheng1" } }, - { state: "APPROVED", user: { login: "bundleowner" } }, - ], - files: [ - { filename: "cmd/pipelines/foo.go" }, - { filename: "bundle/config.go" }, - ], - }); - const core = makeCore(); - const context = makeContext(); - - await runModule({ github, context, core }); - - assert.equal(github._comments.length, 1); - const body = github._comments[0].body; - assert.ok(body.includes("## All ownership groups approved")); - assert.ok(body.includes("Approved by: @jefferycheng1")); - assert.ok(body.includes("Approved by: @bundleowner")); - }); }); From a5b88874fe0983cf30c7e76638852fdcd500e23f Mon Sep 17 00:00:00 2001 From: simon Date: Fri, 10 Apr 2026 10:49:31 +0200 Subject: [PATCH 2/2] Clean up stale pending comments on approval Address review feedback: when a PR transitions from pending to approved, delete any leftover marker comments so the PR does not show a stale "Waiting for approval" comment alongside a green commit status. Co-authored-by: Isaac --- .github/workflows/maintainer-approval.js | 20 ++++++++++++++++++ .github/workflows/maintainer-approval.test.js | 21 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index 1af528c601..7cf7cb4c46 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -366,6 +366,23 @@ function buildSingleDomainPendingComment(sortedScores, dirScores, scoredCount, e const LEGACY_MARKER = ""; +/** + * Delete all marker and legacy marker comments from the PR. + * Used on success paths to clean up stale pending comments. + */ +async function deleteMarkerComments(github, owner, repo, prNumber) { + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: prNumber, + }); + for (const c of comments) { + if (c.body && (c.body.includes(MARKER) || c.body.includes(LEGACY_MARKER))) { + await github.rest.issues.deleteComment({ + owner, repo, comment_id: c.id, + }); + } + } +} + /** * Create or edit the marker comment. Skips the edit if the body is unchanged. * Cleans up duplicate or legacy marker comments, keeping only the first one. @@ -452,6 +469,7 @@ module.exports = async ({ github, context, core }) => { state: "success", description: `Approved by @${approver}`, }); + await deleteMarkerComments(github, owner, repo, prNumber); return; } @@ -468,6 +486,7 @@ module.exports = async ({ github, context, core }) => { state: "success", description: "Approved (maintainer-authored PR)", }); + await deleteMarkerComments(github, owner, repo, prNumber); return; } } @@ -503,6 +522,7 @@ module.exports = async ({ github, context, core }) => { state: "success", description: "All ownership groups approved", }); + await deleteMarkerComments(github, owner, repo, prNumber); return; } diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js index 4ea70e7fc3..24a90c46af 100644 --- a/.github/workflows/maintainer-approval.test.js +++ b/.github/workflows/maintainer-approval.test.js @@ -153,6 +153,27 @@ describe("maintainer-approval", () => { assert.equal(github._updatedComments.length, 0); }); + it("approval cleans up stale pending comment", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "maintainer1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + existingComments: [ + { id: 500, body: "\n## Waiting for approval\n..." }, + ], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._statuses[0].state, "success"); + assert.deepEqual(github._deletedCommentIds, [500]); + assert.equal(github._comments.length, 0); + assert.equal(github._updatedComments.length, 0); + }); + it("maintainer-authored PR with any approval -> success, no comment", async () => { const github = makeGithub({ reviews: [