Skip to content

Commit f5a6fdf

Browse files
Copilotpelikhan
Copilot
and
pelikhan
authored
Add supersede-older-reviews for PR reviews and shift guidance to COMMENT-first defaults (#27662)
* Initial plan * feat: add supersede-older-reviews support for submit PR reviews Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a045c112-653a-4cd4-ac03-99595109b555 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * fix: tighten supersede review filtering and pagination safety Agent-Logs-Url: https://github.com/github/gh-aw/sessions/a045c112-653a-4cd4-ac03-99595109b555 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * fix: scope supersede review matching to workflow call id Agent-Logs-Url: https://github.com/github/gh-aw/sessions/074978ea-6a05-4bb4-b3b5-61863df6ef6b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * chore: align workflow call id naming in supersede tests Agent-Logs-Url: https://github.com/github/gh-aw/sessions/074978ea-6a05-4bb4-b3b5-61863df6ef6b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * fix: make supersede listing best-effort and align docs examples Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0cc78ee4-76d7-404a-8949-d4625e88d412 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>
1 parent 4bc668c commit f5a6fdf

15 files changed

Lines changed: 405 additions & 7 deletions

‎actions/setup/js/pr_review_buffer.cjs‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@
2222
const { generateFooterWithMessages } = require("./messages_footer.cjs");
2323
const { getErrorMessage } = require("./error_helpers.cjs");
2424
const { isStagedMode } = require("./safe_output_helpers.cjs");
25+
const { generateWorkflowCallIdMarker, matchesWorkflowId } = require("./generate_footer.cjs");
26+
27+
const SUPERSEDE_REVIEW_MESSAGE = "Superseded by updated review from same workflow.";
28+
const MAX_SUPERSEDE_REVIEW_PAGES = 10;
2529

2630
/**
2731
* @typedef {Object} BufferedComment
@@ -71,6 +75,9 @@ function createReviewBuffer() {
7175

7276
/** @type {boolean} Staged mode: when true, preview review without submitting (set via setStaged(), reset on buffer clear) */
7377
let stagedMode = false;
78+
79+
/** @type {boolean} When true, dismiss older same-workflow REQUEST_CHANGES reviews after posting a replacement review. */
80+
let supersedeOlderReviews = false;
7481
/**
7582
* Add a validated comment to the buffer.
7683
* Rejects comments targeting a different repo/PR than the first comment.
@@ -172,6 +179,17 @@ function createReviewBuffer() {
172179
}
173180
}
174181

182+
/**
183+
* Enable/disable superseding older same-workflow REQUEST_CHANGES reviews.
184+
* @param {boolean} value - Whether supersede behavior is enabled
185+
*/
186+
function setSupersedeOlderReviews(value) {
187+
supersedeOlderReviews = value === true;
188+
if (supersedeOlderReviews) {
189+
core.info("PR review supersede mode enabled");
190+
}
191+
}
192+
175193
/**
176194
* Check if there are buffered comments to submit.
177195
* @returns {boolean}
@@ -244,6 +262,11 @@ function createReviewBuffer() {
244262
footerContext.triggeringPRNumber,
245263
footerContext.triggeringDiscussionNumber
246264
);
265+
266+
const callerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID || "";
267+
if (callerWorkflowId) {
268+
body += "\n" + generateWorkflowCallIdMarker(callerWorkflowId);
269+
}
247270
}
248271

249272
// Build comments array for the API
@@ -322,8 +345,82 @@ function createReviewBuffer() {
322345
requestParams.body = body;
323346
}
324347

348+
/**
349+
* Dismiss older REQUEST_CHANGES reviews from the same workflow after posting a replacement review.
350+
* This is best-effort: failures are logged as warnings and do not fail the current review submission.
351+
* @param {number} currentReviewId
352+
*/
353+
async function maybeSupersedeOlderReviews(currentReviewId) {
354+
if (!supersedeOlderReviews) {
355+
return;
356+
}
357+
358+
const workflowId = process.env.GH_AW_WORKFLOW_ID || "";
359+
const workflowCallId = process.env.GH_AW_CALLER_WORKFLOW_ID || "";
360+
if (!workflowId && !workflowCallId) {
361+
core.warning("supersede-older-reviews is enabled but neither GH_AW_WORKFLOW_ID nor GH_AW_CALLER_WORKFLOW_ID is set. Skipping stale review dismissal.");
362+
return;
363+
}
364+
const workflowCallMarker = workflowCallId ? generateWorkflowCallIdMarker(workflowCallId) : "";
365+
try {
366+
/** @type {Array<{id: number, state?: string, user?: {login?: string, type?: string}, body?: string}>} */
367+
const reviews = [];
368+
let page = 1;
369+
const perPage = 100;
370+
while (page <= MAX_SUPERSEDE_REVIEW_PAGES) {
371+
const { data } = await github.rest.pulls.listReviews({
372+
owner: repoParts.owner,
373+
repo: repoParts.repo,
374+
pull_number: pullRequestNumber,
375+
per_page: perPage,
376+
page,
377+
});
378+
379+
if (!Array.isArray(data) || data.length === 0) {
380+
break;
381+
}
382+
reviews.push(...data);
383+
if (data.length < perPage) {
384+
break;
385+
}
386+
page++;
387+
}
388+
if (page > MAX_SUPERSEDE_REVIEW_PAGES) {
389+
core.warning(`supersede-older-reviews reached pagination safety limit (${MAX_SUPERSEDE_REVIEW_PAGES} pages).`);
390+
}
391+
392+
const staleReviews = reviews.filter(review => {
393+
if (!review || review.id === currentReviewId) return false;
394+
if (review.state !== "CHANGES_REQUESTED") return false;
395+
if (review.user?.type !== "Bot") return false;
396+
if (workflowCallMarker) {
397+
return review.body?.includes(workflowCallMarker) || false;
398+
}
399+
return matchesWorkflowId(review.body, workflowId);
400+
});
401+
402+
for (const staleReview of staleReviews) {
403+
try {
404+
await github.rest.pulls.dismissReview({
405+
owner: repoParts.owner,
406+
repo: repoParts.repo,
407+
pull_number: pullRequestNumber,
408+
review_id: staleReview.id,
409+
message: SUPERSEDE_REVIEW_MESSAGE,
410+
});
411+
core.info(`Dismissed superseded review #${staleReview.id}`);
412+
} catch (dismissError) {
413+
core.warning(`Failed to dismiss stale review #${staleReview.id}: ${getErrorMessage(dismissError)}`);
414+
}
415+
}
416+
} catch (listOrSupersedeError) {
417+
core.warning(`Failed to supersede older reviews: ${getErrorMessage(listOrSupersedeError)}`);
418+
}
419+
}
420+
325421
try {
326422
const { data: review } = await github.rest.pulls.createReview(requestParams);
423+
await maybeSupersedeOlderReviews(review.id);
327424

328425
core.info(`Created PR review #${review.id}: ${review.html_url}`);
329426

@@ -348,6 +445,7 @@ function createReviewBuffer() {
348445
try {
349446
requestParams.event = "COMMENT";
350447
const { data: review } = await github.rest.pulls.createReview(requestParams);
448+
await maybeSupersedeOlderReviews(review.id);
351449
core.info(`Created PR review #${review.id}: ${review.html_url}`);
352450
return {
353451
success: true,
@@ -375,6 +473,7 @@ function createReviewBuffer() {
375473
const bodyOnlyParams = { ...requestParams };
376474
delete bodyOnlyParams.comments;
377475
const { data: review } = await github.rest.pulls.createReview(bodyOnlyParams);
476+
await maybeSupersedeOlderReviews(review.id);
378477
core.info(`Created PR review #${review.id} (body-only fallback): ${review.html_url}`);
379478
return {
380479
success: true,
@@ -423,6 +522,7 @@ function createReviewBuffer() {
423522
setFooterMode,
424523
setIncludeFooter: setFooterMode, // Backward compatibility alias
425524
setStaged,
525+
setSupersedeOlderReviews,
426526
hasBufferedComments,
427527
hasReviewMetadata,
428528
getBufferedCount,

‎actions/setup/js/pr_review_buffer.test.cjs‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ const mockGithub = {
1313
rest: {
1414
pulls: {
1515
createReview: vi.fn(),
16+
listReviews: vi.fn(),
17+
dismissReview: vi.fn(),
1618
},
1719
},
1820
};
@@ -394,6 +396,46 @@ describe("pr_review_buffer (factory pattern)", () => {
394396
expect(callArgs.body).toContain("test-workflow");
395397
});
396398

399+
it("should append workflow-call-id marker to review body when available", async () => {
400+
const previousCallerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID;
401+
process.env.GH_AW_CALLER_WORKFLOW_ID = "owner/repo/CallerA";
402+
try {
403+
buffer.addComment({ path: "test.js", line: 1, body: "comment" });
404+
buffer.setReviewMetadata("Review body", "COMMENT");
405+
buffer.setReviewContext({
406+
repo: "owner/repo",
407+
repoParts: { owner: "owner", repo: "repo" },
408+
pullRequestNumber: 42,
409+
pullRequest: { head: { sha: "abc123" } },
410+
});
411+
buffer.setFooterContext({
412+
workflowName: "test-workflow",
413+
runUrl: "https://github.com/owner/repo/actions/runs/123",
414+
workflowSource: "owner/repo/workflows/test.md@v1",
415+
workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md",
416+
});
417+
418+
mockGithub.rest.pulls.createReview.mockResolvedValue({
419+
data: {
420+
id: 405,
421+
html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-405",
422+
},
423+
});
424+
425+
const result = await buffer.submitReview();
426+
expect(result.success).toBe(true);
427+
428+
const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0];
429+
expect(callArgs.body).toContain("<!-- gh-aw-workflow-call-id: owner/repo/CallerA -->");
430+
} finally {
431+
if (previousCallerWorkflowId === undefined) {
432+
delete process.env.GH_AW_CALLER_WORKFLOW_ID;
433+
} else {
434+
process.env.GH_AW_CALLER_WORKFLOW_ID = previousCallerWorkflowId;
435+
}
436+
}
437+
});
438+
397439
it("should skip footer when setIncludeFooter('none') is called", async () => {
398440
buffer.addComment({ path: "test.js", line: 1, body: "comment" });
399441
buffer.setReviewMetadata("Review body", "COMMENT");
@@ -574,6 +616,136 @@ describe("pr_review_buffer (factory pattern)", () => {
574616
expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(2);
575617
});
576618

619+
it("should dismiss older reviews matching workflow-call-id when supersede mode is enabled", async () => {
620+
const previousWorkflowId = process.env.GH_AW_WORKFLOW_ID;
621+
const previousCallerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID;
622+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
623+
process.env.GH_AW_CALLER_WORKFLOW_ID = "owner/repo/CallerA";
624+
try {
625+
buffer.setSupersedeOlderReviews(true);
626+
buffer.setReviewMetadata("Updated review", "COMMENT");
627+
buffer.setReviewContext({
628+
repo: "owner/repo",
629+
repoParts: { owner: "owner", repo: "repo" },
630+
pullRequestNumber: 42,
631+
pullRequest: { head: { sha: "abc123" } },
632+
});
633+
634+
mockGithub.rest.pulls.createReview.mockResolvedValue({
635+
data: {
636+
id: 900,
637+
html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-900",
638+
},
639+
});
640+
mockGithub.rest.pulls.listReviews.mockResolvedValue({
641+
data: [
642+
{ id: 100, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-call-id: owner/repo/CallerA -->\nOld blocking review" },
643+
{ id: 101, state: "CHANGES_REQUESTED", user: { login: "human-user", type: "User" }, body: "<!-- gh-aw-workflow-call-id: owner/repo/CallerA -->" },
644+
{ id: 102, state: "APPROVED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-call-id: owner/repo/CallerA -->" },
645+
{ id: 103, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-call-id: owner/repo/CallerB -->" },
646+
{ id: 104, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-id: test-workflow -->" },
647+
],
648+
});
649+
mockGithub.rest.pulls.dismissReview.mockResolvedValue({ data: {} });
650+
651+
const result = await buffer.submitReview();
652+
653+
expect(result.success).toBe(true);
654+
expect(mockGithub.rest.pulls.listReviews).toHaveBeenCalledTimes(1);
655+
expect(mockGithub.rest.pulls.dismissReview).toHaveBeenCalledTimes(1);
656+
expect(mockGithub.rest.pulls.dismissReview).toHaveBeenCalledWith({
657+
owner: "owner",
658+
repo: "repo",
659+
pull_number: 42,
660+
review_id: 100,
661+
message: "Superseded by updated review from same workflow.",
662+
});
663+
} finally {
664+
if (previousWorkflowId === undefined) {
665+
delete process.env.GH_AW_WORKFLOW_ID;
666+
} else {
667+
process.env.GH_AW_WORKFLOW_ID = previousWorkflowId;
668+
}
669+
if (previousCallerWorkflowId === undefined) {
670+
delete process.env.GH_AW_CALLER_WORKFLOW_ID;
671+
} else {
672+
process.env.GH_AW_CALLER_WORKFLOW_ID = previousCallerWorkflowId;
673+
}
674+
}
675+
});
676+
677+
it("should warn and continue when stale review dismissal fails", async () => {
678+
const previousWorkflowId = process.env.GH_AW_WORKFLOW_ID;
679+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
680+
try {
681+
buffer.setSupersedeOlderReviews(true);
682+
buffer.setReviewMetadata("Updated review", "COMMENT");
683+
buffer.setReviewContext({
684+
repo: "owner/repo",
685+
repoParts: { owner: "owner", repo: "repo" },
686+
pullRequestNumber: 42,
687+
pullRequest: { head: { sha: "abc123" } },
688+
});
689+
690+
mockGithub.rest.pulls.createReview.mockResolvedValue({
691+
data: {
692+
id: 901,
693+
html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-901",
694+
},
695+
});
696+
mockGithub.rest.pulls.listReviews.mockResolvedValue({
697+
data: [{ id: 200, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-id: test-workflow -->" }],
698+
});
699+
mockGithub.rest.pulls.dismissReview.mockRejectedValue(new Error("permission denied"));
700+
701+
const result = await buffer.submitReview();
702+
703+
expect(result.success).toBe(true);
704+
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to dismiss stale review #200"));
705+
} finally {
706+
if (previousWorkflowId === undefined) {
707+
delete process.env.GH_AW_WORKFLOW_ID;
708+
} else {
709+
process.env.GH_AW_WORKFLOW_ID = previousWorkflowId;
710+
}
711+
}
712+
});
713+
714+
it("should warn and continue when stale review listing fails", async () => {
715+
const previousWorkflowId = process.env.GH_AW_WORKFLOW_ID;
716+
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
717+
try {
718+
buffer.setSupersedeOlderReviews(true);
719+
buffer.setReviewMetadata("Updated review", "COMMENT");
720+
buffer.setReviewContext({
721+
repo: "owner/repo",
722+
repoParts: { owner: "owner", repo: "repo" },
723+
pullRequestNumber: 42,
724+
pullRequest: { head: { sha: "abc123" } },
725+
});
726+
727+
mockGithub.rest.pulls.createReview.mockResolvedValue({
728+
data: {
729+
id: 902,
730+
html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-902",
731+
},
732+
});
733+
mockGithub.rest.pulls.listReviews.mockRejectedValue(new Error("rate limited"));
734+
735+
const result = await buffer.submitReview();
736+
737+
expect(result.success).toBe(true);
738+
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to supersede older reviews"));
739+
expect(mockGithub.rest.pulls.dismissReview).not.toHaveBeenCalled();
740+
} finally {
741+
if (previousWorkflowId === undefined) {
742+
delete process.env.GH_AW_WORKFLOW_ID;
743+
} else {
744+
process.env.GH_AW_WORKFLOW_ID = previousWorkflowId;
745+
}
746+
}
747+
});
748+
577749
it("should handle API errors gracefully", async () => {
578750
buffer.addComment({ path: "test.js", line: 1, body: "comment" });
579751
buffer.setReviewContext({

‎actions/setup/js/safe_outputs_tools.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@
360360
},
361361
{
362362
"name": "submit_pull_request_review",
363-
"description": "Submit a pull request review with a status decision. All create_pull_request_review_comment outputs are automatically collected and included as inline comments in this review. Use APPROVE to approve the PR, REQUEST_CHANGES to request changes, or COMMENT for general feedback without a decision. If you don't call this tool, review comments are still submitted as a COMMENT review.",
363+
"description": "Submit a pull request review with a status decision. All create_pull_request_review_comment outputs are automatically collected and included as inline comments in this review. Preferred default: use COMMENT for informative, non-blocking bot feedback. Use REQUEST_CHANGES only when merge-blocking is intentionally required. If you don't call this tool, review comments are still submitted as a COMMENT review.",
364364
"inputSchema": {
365365
"type": "object",
366366
"properties": {

‎actions/setup/js/submit_pr_review.cjs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const { resolveTarget, isStagedMode, logStagedPreviewInfo } = require("./safe_ou
99
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
1010
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
1111
const { getErrorMessage } = require("./error_helpers.cjs");
12+
const { parseBoolTemplatable } = require("./templatable.cjs");
1213

1314
/** @type {string} Safe output type handled by this module */
1415
const HANDLER_TYPE = "submit_pull_request_review";
@@ -30,6 +31,7 @@ async function main(config = {}) {
3031
const maxCount = config.max || 1;
3132
const targetConfig = config.target || "triggering";
3233
const buffer = config._prReviewBuffer;
34+
const supersedeOlderReviews = parseBoolTemplatable(config.supersede_older_reviews, false);
3335
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);
3436
const githubClient = await createAuthenticatedGitHubClient(config);
3537

@@ -59,6 +61,12 @@ async function main(config = {}) {
5961
if (isStagedMode(config)) {
6062
logStagedPreviewInfo("PR review will be previewed without being submitted");
6163
}
64+
if (supersedeOlderReviews) {
65+
core.warning("submit_pull_request_review: supersede-older-reviews is best-effort. Prefer allowed-events: [COMMENT] by default and use REQUEST_CHANGES only when merge-blocking is required.");
66+
if (typeof buffer.setSupersedeOlderReviews === "function") {
67+
buffer.setSupersedeOlderReviews(true);
68+
}
69+
}
6270

6371
let processedCount = 0;
6472

0 commit comments

Comments
 (0)