Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions actions/setup/js/pr_review_buffer.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@
const { generateFooterWithMessages } = require("./messages_footer.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");
const { isStagedMode } = require("./safe_output_helpers.cjs");
const { generateWorkflowCallIdMarker, matchesWorkflowId } = require("./generate_footer.cjs");

const SUPERSEDE_REVIEW_MESSAGE = "Superseded by updated review from same workflow.";
const MAX_SUPERSEDE_REVIEW_PAGES = 10;

/**
* @typedef {Object} BufferedComment
Expand Down Expand Up @@ -71,6 +75,9 @@ function createReviewBuffer() {

/** @type {boolean} Staged mode: when true, preview review without submitting (set via setStaged(), reset on buffer clear) */
let stagedMode = false;

/** @type {boolean} When true, dismiss older same-workflow REQUEST_CHANGES reviews after posting a replacement review. */
let supersedeOlderReviews = false;
/**
* Add a validated comment to the buffer.
* Rejects comments targeting a different repo/PR than the first comment.
Expand Down Expand Up @@ -172,6 +179,17 @@ function createReviewBuffer() {
}
}

/**
* Enable/disable superseding older same-workflow REQUEST_CHANGES reviews.
* @param {boolean} value - Whether supersede behavior is enabled
*/
function setSupersedeOlderReviews(value) {
supersedeOlderReviews = value === true;
if (supersedeOlderReviews) {
core.info("PR review supersede mode enabled");
}
}

/**
* Check if there are buffered comments to submit.
* @returns {boolean}
Expand Down Expand Up @@ -244,6 +262,11 @@ function createReviewBuffer() {
footerContext.triggeringPRNumber,
footerContext.triggeringDiscussionNumber
);

const callerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID || "";
if (callerWorkflowId) {
body += "\n" + generateWorkflowCallIdMarker(callerWorkflowId);
}
}

// Build comments array for the API
Expand Down Expand Up @@ -322,8 +345,82 @@ function createReviewBuffer() {
requestParams.body = body;
}

/**
* Dismiss older REQUEST_CHANGES reviews from the same workflow after posting a replacement review.
* This is best-effort: failures are logged as warnings and do not fail the current review submission.
* @param {number} currentReviewId
*/
async function maybeSupersedeOlderReviews(currentReviewId) {
if (!supersedeOlderReviews) {
return;
}

const workflowId = process.env.GH_AW_WORKFLOW_ID || "";
const workflowCallId = process.env.GH_AW_CALLER_WORKFLOW_ID || "";
if (!workflowId && !workflowCallId) {
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.");
return;
}
const workflowCallMarker = workflowCallId ? generateWorkflowCallIdMarker(workflowCallId) : "";
try {
/** @type {Array<{id: number, state?: string, user?: {login?: string, type?: string}, body?: string}>} */
const reviews = [];
let page = 1;
const perPage = 100;
while (page <= MAX_SUPERSEDE_REVIEW_PAGES) {
const { data } = await github.rest.pulls.listReviews({
owner: repoParts.owner,
repo: repoParts.repo,
pull_number: pullRequestNumber,
per_page: perPage,
page,
});

if (!Array.isArray(data) || data.length === 0) {
break;
}
reviews.push(...data);
if (data.length < perPage) {
break;
}
page++;
}
if (page > MAX_SUPERSEDE_REVIEW_PAGES) {
core.warning(`supersede-older-reviews reached pagination safety limit (${MAX_SUPERSEDE_REVIEW_PAGES} pages).`);
}

const staleReviews = reviews.filter(review => {
if (!review || review.id === currentReviewId) return false;
if (review.state !== "CHANGES_REQUESTED") return false;
if (review.user?.type !== "Bot") return false;
if (workflowCallMarker) {
return review.body?.includes(workflowCallMarker) || false;
}
return matchesWorkflowId(review.body, workflowId);
});

for (const staleReview of staleReviews) {
try {
await github.rest.pulls.dismissReview({
owner: repoParts.owner,
repo: repoParts.repo,
pull_number: pullRequestNumber,
review_id: staleReview.id,
message: SUPERSEDE_REVIEW_MESSAGE,
});
core.info(`Dismissed superseded review #${staleReview.id}`);
} catch (dismissError) {
core.warning(`Failed to dismiss stale review #${staleReview.id}: ${getErrorMessage(dismissError)}`);
}
}
} catch (listOrSupersedeError) {
core.warning(`Failed to supersede older reviews: ${getErrorMessage(listOrSupersedeError)}`);
}
}

try {
const { data: review } = await github.rest.pulls.createReview(requestParams);
await maybeSupersedeOlderReviews(review.id);

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

Expand All @@ -348,6 +445,7 @@ function createReviewBuffer() {
try {
requestParams.event = "COMMENT";
const { data: review } = await github.rest.pulls.createReview(requestParams);
await maybeSupersedeOlderReviews(review.id);
core.info(`Created PR review #${review.id}: ${review.html_url}`);
return {
success: true,
Expand Down Expand Up @@ -375,6 +473,7 @@ function createReviewBuffer() {
const bodyOnlyParams = { ...requestParams };
delete bodyOnlyParams.comments;
const { data: review } = await github.rest.pulls.createReview(bodyOnlyParams);
await maybeSupersedeOlderReviews(review.id);
core.info(`Created PR review #${review.id} (body-only fallback): ${review.html_url}`);
return {
success: true,
Expand Down Expand Up @@ -423,6 +522,7 @@ function createReviewBuffer() {
setFooterMode,
setIncludeFooter: setFooterMode, // Backward compatibility alias
setStaged,
setSupersedeOlderReviews,
hasBufferedComments,
hasReviewMetadata,
getBufferedCount,
Expand Down
172 changes: 172 additions & 0 deletions actions/setup/js/pr_review_buffer.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const mockGithub = {
rest: {
pulls: {
createReview: vi.fn(),
listReviews: vi.fn(),
dismissReview: vi.fn(),
},
},
};
Expand Down Expand Up @@ -394,6 +396,46 @@ describe("pr_review_buffer (factory pattern)", () => {
expect(callArgs.body).toContain("test-workflow");
});

it("should append workflow-call-id marker to review body when available", async () => {
const previousCallerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID;
process.env.GH_AW_CALLER_WORKFLOW_ID = "owner/repo/CallerA";
try {
buffer.addComment({ path: "test.js", line: 1, body: "comment" });
buffer.setReviewMetadata("Review body", "COMMENT");
buffer.setReviewContext({
repo: "owner/repo",
repoParts: { owner: "owner", repo: "repo" },
pullRequestNumber: 42,
pullRequest: { head: { sha: "abc123" } },
});
buffer.setFooterContext({
workflowName: "test-workflow",
runUrl: "https://github.com/owner/repo/actions/runs/123",
workflowSource: "owner/repo/workflows/test.md@v1",
workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md",
});

mockGithub.rest.pulls.createReview.mockResolvedValue({
data: {
id: 405,
html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-405",
},
});

const result = await buffer.submitReview();
expect(result.success).toBe(true);

const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0];
expect(callArgs.body).toContain("<!-- gh-aw-workflow-call-id: owner/repo/CallerA -->");
} finally {
if (previousCallerWorkflowId === undefined) {
delete process.env.GH_AW_CALLER_WORKFLOW_ID;
} else {
process.env.GH_AW_CALLER_WORKFLOW_ID = previousCallerWorkflowId;
}
}
});

it("should skip footer when setIncludeFooter('none') is called", async () => {
buffer.addComment({ path: "test.js", line: 1, body: "comment" });
buffer.setReviewMetadata("Review body", "COMMENT");
Expand Down Expand Up @@ -574,6 +616,136 @@ describe("pr_review_buffer (factory pattern)", () => {
expect(mockGithub.rest.pulls.createReview).toHaveBeenCalledTimes(2);
});

it("should dismiss older reviews matching workflow-call-id when supersede mode is enabled", async () => {
const previousWorkflowId = process.env.GH_AW_WORKFLOW_ID;
const previousCallerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID;
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
process.env.GH_AW_CALLER_WORKFLOW_ID = "owner/repo/CallerA";
try {
buffer.setSupersedeOlderReviews(true);
buffer.setReviewMetadata("Updated review", "COMMENT");
buffer.setReviewContext({
repo: "owner/repo",
repoParts: { owner: "owner", repo: "repo" },
pullRequestNumber: 42,
pullRequest: { head: { sha: "abc123" } },
});

mockGithub.rest.pulls.createReview.mockResolvedValue({
data: {
id: 900,
html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-900",
},
});
mockGithub.rest.pulls.listReviews.mockResolvedValue({
data: [
{ id: 100, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-call-id: owner/repo/CallerA -->\nOld blocking review" },
{ id: 101, state: "CHANGES_REQUESTED", user: { login: "human-user", type: "User" }, body: "<!-- gh-aw-workflow-call-id: owner/repo/CallerA -->" },
{ id: 102, state: "APPROVED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-call-id: owner/repo/CallerA -->" },
{ id: 103, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-call-id: owner/repo/CallerB -->" },
{ id: 104, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-id: test-workflow -->" },
],
});
mockGithub.rest.pulls.dismissReview.mockResolvedValue({ data: {} });

const result = await buffer.submitReview();

expect(result.success).toBe(true);
expect(mockGithub.rest.pulls.listReviews).toHaveBeenCalledTimes(1);
expect(mockGithub.rest.pulls.dismissReview).toHaveBeenCalledTimes(1);
expect(mockGithub.rest.pulls.dismissReview).toHaveBeenCalledWith({
owner: "owner",
repo: "repo",
pull_number: 42,
review_id: 100,
message: "Superseded by updated review from same workflow.",
});
} finally {
if (previousWorkflowId === undefined) {
delete process.env.GH_AW_WORKFLOW_ID;
} else {
process.env.GH_AW_WORKFLOW_ID = previousWorkflowId;
}
if (previousCallerWorkflowId === undefined) {
delete process.env.GH_AW_CALLER_WORKFLOW_ID;
} else {
process.env.GH_AW_CALLER_WORKFLOW_ID = previousCallerWorkflowId;
}
}
});

it("should warn and continue when stale review dismissal fails", async () => {
const previousWorkflowId = process.env.GH_AW_WORKFLOW_ID;
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
try {
buffer.setSupersedeOlderReviews(true);
buffer.setReviewMetadata("Updated review", "COMMENT");
buffer.setReviewContext({
repo: "owner/repo",
repoParts: { owner: "owner", repo: "repo" },
pullRequestNumber: 42,
pullRequest: { head: { sha: "abc123" } },
});

mockGithub.rest.pulls.createReview.mockResolvedValue({
data: {
id: 901,
html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-901",
},
});
mockGithub.rest.pulls.listReviews.mockResolvedValue({
data: [{ id: 200, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "<!-- gh-aw-workflow-id: test-workflow -->" }],
});
mockGithub.rest.pulls.dismissReview.mockRejectedValue(new Error("permission denied"));

const result = await buffer.submitReview();

expect(result.success).toBe(true);
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to dismiss stale review #200"));
} finally {
if (previousWorkflowId === undefined) {
delete process.env.GH_AW_WORKFLOW_ID;
} else {
process.env.GH_AW_WORKFLOW_ID = previousWorkflowId;
}
}
});

it("should warn and continue when stale review listing fails", async () => {
const previousWorkflowId = process.env.GH_AW_WORKFLOW_ID;
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
try {
buffer.setSupersedeOlderReviews(true);
buffer.setReviewMetadata("Updated review", "COMMENT");
buffer.setReviewContext({
repo: "owner/repo",
repoParts: { owner: "owner", repo: "repo" },
pullRequestNumber: 42,
pullRequest: { head: { sha: "abc123" } },
});

mockGithub.rest.pulls.createReview.mockResolvedValue({
data: {
id: 902,
html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-902",
},
});
mockGithub.rest.pulls.listReviews.mockRejectedValue(new Error("rate limited"));

const result = await buffer.submitReview();

expect(result.success).toBe(true);
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to supersede older reviews"));
expect(mockGithub.rest.pulls.dismissReview).not.toHaveBeenCalled();
} finally {
if (previousWorkflowId === undefined) {
delete process.env.GH_AW_WORKFLOW_ID;
} else {
process.env.GH_AW_WORKFLOW_ID = previousWorkflowId;
}
}
});

it("should handle API errors gracefully", async () => {
buffer.addComment({ path: "test.js", line: 1, body: "comment" });
buffer.setReviewContext({
Expand Down
2 changes: 1 addition & 1 deletion actions/setup/js/safe_outputs_tools.json
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@
},
{
"name": "submit_pull_request_review",
"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.",
"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.",
"inputSchema": {
"type": "object",
"properties": {
Expand Down
8 changes: 8 additions & 0 deletions actions/setup/js/submit_pr_review.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const { resolveTarget, isStagedMode, logStagedPreviewInfo } = require("./safe_ou
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");
const { parseBoolTemplatable } = require("./templatable.cjs");

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

Expand Down Expand Up @@ -59,6 +61,12 @@ async function main(config = {}) {
if (isStagedMode(config)) {
logStagedPreviewInfo("PR review will be previewed without being submitted");
}
if (supersedeOlderReviews) {
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.");
if (typeof buffer.setSupersedeOlderReviews === "function") {
buffer.setSupersedeOlderReviews(true);
}
}

let processedCount = 0;

Expand Down
Loading
Loading