diff --git a/actions/setup/js/pr_review_buffer.cjs b/actions/setup/js/pr_review_buffer.cjs index a848407fc2d..a53515cf87c 100644 --- a/actions/setup/js/pr_review_buffer.cjs +++ b/actions/setup/js/pr_review_buffer.cjs @@ -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 @@ -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. @@ -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} @@ -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 @@ -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}`); @@ -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, @@ -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, @@ -423,6 +522,7 @@ function createReviewBuffer() { setFooterMode, setIncludeFooter: setFooterMode, // Backward compatibility alias setStaged, + setSupersedeOlderReviews, hasBufferedComments, hasReviewMetadata, getBufferedCount, diff --git a/actions/setup/js/pr_review_buffer.test.cjs b/actions/setup/js/pr_review_buffer.test.cjs index 35ac1877bf7..b4b36e27e46 100644 --- a/actions/setup/js/pr_review_buffer.test.cjs +++ b/actions/setup/js/pr_review_buffer.test.cjs @@ -13,6 +13,8 @@ const mockGithub = { rest: { pulls: { createReview: vi.fn(), + listReviews: vi.fn(), + dismissReview: vi.fn(), }, }, }; @@ -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(""); + } 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"); @@ -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: "\nOld blocking review" }, + { id: 101, state: "CHANGES_REQUESTED", user: { login: "human-user", type: "User" }, body: "" }, + { id: 102, state: "APPROVED", user: { login: "github-actions[bot]", type: "Bot" }, body: "" }, + { id: 103, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "" }, + { id: 104, state: "CHANGES_REQUESTED", user: { login: "github-actions[bot]", type: "Bot" }, body: "" }, + ], + }); + 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: "" }], + }); + 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({ diff --git a/actions/setup/js/safe_outputs_tools.json b/actions/setup/js/safe_outputs_tools.json index f674643bb08..d4a2f9c7ae0 100644 --- a/actions/setup/js/safe_outputs_tools.json +++ b/actions/setup/js/safe_outputs_tools.json @@ -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": { diff --git a/actions/setup/js/submit_pr_review.cjs b/actions/setup/js/submit_pr_review.cjs index 6769fa4f4ae..f45ce974a81 100644 --- a/actions/setup/js/submit_pr_review.cjs +++ b/actions/setup/js/submit_pr_review.cjs @@ -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"; @@ -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); @@ -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; diff --git a/actions/setup/js/submit_pr_review.test.cjs b/actions/setup/js/submit_pr_review.test.cjs index e39f925bef1..572a7c91378 100644 --- a/actions/setup/js/submit_pr_review.test.cjs +++ b/actions/setup/js/submit_pr_review.test.cjs @@ -94,6 +94,21 @@ describe("submit_pr_review (Handler Factory Architecture)", () => { expect(result.error).toContain("No PR review buffer available"); }); + it("should enable supersede mode on review buffer when configured", async () => { + const { main } = require("./submit_pr_review.cjs"); + const localBuffer = createReviewBuffer(); + const supersedeSpy = vi.spyOn(localBuffer, "setSupersedeOlderReviews"); + + await main({ + max: 1, + _prReviewBuffer: localBuffer, + supersede_older_reviews: true, + }); + + expect(supersedeSpy).toHaveBeenCalledWith(true); + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("supersede-older-reviews is best-effort")); + }); + it("should set review metadata for APPROVE event", async () => { const message = { type: "submit_pull_request_review", diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index 37e2e549598..6931997c17e 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -3775,6 +3775,13 @@ safe-outputs: allowed-events: [] # Array of strings + # When true, after posting a replacement review this workflow dismisses older + # REQUEST_CHANGES reviews previously posted by the same workflow on the same pull + # request. This is best-effort and requires workflow markers in prior review + # bodies. + # (optional) + supersede-older-reviews: true + # GitHub token to use for this specific output type. Overrides global github-token # if specified. # (optional) diff --git a/docs/src/content/docs/reference/glossary.md b/docs/src/content/docs/reference/glossary.md index d6ec2dda28a..9c522b02da1 100644 --- a/docs/src/content/docs/reference/glossary.md +++ b/docs/src/content/docs/reference/glossary.md @@ -215,7 +215,7 @@ A field on `create-pull-request` and `push-to-pull-request-branch` safe outputs ### Allowed Events (`allowed-events:`) -A field on `submit-pull-request-review:` safe outputs that restricts which PR review event types the agent may submit. Accepts an array of `APPROVE`, `COMMENT`, and `REQUEST_CHANGES`. When set, the safe-outputs handler rejects any review event not in the list, providing infrastructure-level enforcement regardless of what the agent attempts to output. If omitted, all three event types are allowed. Example: `allowed-events: [COMMENT, REQUEST_CHANGES]` prevents the agent from approving PRs. See [Safe Outputs Reference](/gh-aw/reference/safe-outputs/#submit-pr-review-submit-pull-request-review). +A field on `submit-pull-request-review:` safe outputs that restricts which PR review event types the agent may submit. Accepts an array of `APPROVE`, `COMMENT`, and `REQUEST_CHANGES`. When set, the safe-outputs handler rejects any review event not in the list, providing infrastructure-level enforcement regardless of what the agent attempts to output. If omitted, all three event types are allowed. Preferred default for bot reviews: `allowed-events: [COMMENT]`. Example: `allowed-events: [COMMENT, REQUEST_CHANGES]` prevents the agent from approving PRs. See [Safe Outputs Reference](/gh-aw/reference/safe-outputs/#submit-pr-review-submit-pull-request-review). ### Allowed Files diff --git a/docs/src/content/docs/reference/safe-outputs-pull-requests.md b/docs/src/content/docs/reference/safe-outputs-pull-requests.md index af777a6be7c..68a28e72a58 100644 --- a/docs/src/content/docs/reference/safe-outputs-pull-requests.md +++ b/docs/src/content/docs/reference/safe-outputs-pull-requests.md @@ -11,6 +11,7 @@ This page is the primary reference for pull-request-focused safe outputs: - [`update-pull-request`](#pull-request-updates-update-pull-request) - [`close-pull-request`](#close-pull-request-close-pull-request) - [`create-pull-request-review-comment`](#pr-review-comments-create-pull-request-review-comment) +- [`submit-pull-request-review`](#submit-pr-review-submit-pull-request-review) - [`reply-to-pull-request-review-comment`](#reply-to-pr-review-comment-reply-to-pull-request-review-comment) - [`resolve-pull-request-review-thread`](#resolve-pr-review-thread-resolve-pull-request-review-thread) - [`push-to-pull-request-branch`](#push-to-pr-branch-push-to-pull-request-branch) @@ -172,6 +173,26 @@ safe-outputs: When `target: "*"` is configured, the agent must supply `pull_request_number` in each `create_pull_request_review_comment` tool call to identify which PR to comment on — omitting it will cause the comment to fail. For cross-repository scenarios, the agent can also supply `repo` (in `owner/repo` format) to route the comment to a PR in a different repository; the value must match `target-repo` or appear in `allowed-repos`. +## Submit PR Review (`submit-pull-request-review:`) + +Submits a consolidated pull request review. Inline comments buffered by `create-pull-request-review-comment` are included automatically. + +```yaml wrap +safe-outputs: + submit-pull-request-review: + max: 1 + allowed-events: [COMMENT, REQUEST_CHANGES] # include REQUEST_CHANGES when superseding older blocking reviews + supersede-older-reviews: true # dismiss older same-workflow REQUEST_CHANGES reviews after replacement + target: "triggering" # or "*", or explicit PR number + target-repo: "owner/repo" # cross-repository + allowed-repos: ["org/repo1"] # additional allowed repositories + footer: "always" # "always", "none", or "if-body" +``` + +Use `allowed-events` to control review decisions (`APPROVE`, `COMMENT`, `REQUEST_CHANGES`). Prefer `allowed-events: [COMMENT]` by default so bot reviews remain informative and non-blocking. + +When you intentionally allow `REQUEST_CHANGES`, set `supersede-older-reviews: true` to dismiss older blocking reviews from the same workflow after posting a replacement review. This behavior is best-effort. + ## Reply to PR Review Comment (`reply-to-pull-request-review-comment:`) Replies to existing review comments on pull requests. Use this to respond to reviewer feedback, answer questions, or acknowledge comments. The `comment_id` must be the numeric ID of an existing review comment. diff --git a/docs/src/content/docs/reference/safe-outputs-specification.md b/docs/src/content/docs/reference/safe-outputs-specification.md index 1622e140bce..d5c66d63dae 100644 --- a/docs/src/content/docs/reference/safe-outputs-specification.md +++ b/docs/src/content/docs/reference/safe-outputs-specification.md @@ -1565,6 +1565,8 @@ submit-pull-request-review: target: "triggering" | "*" | # Required when not in pull_request trigger target-repo: owner/repo # Cross-repository target allowed-repos: [...] # Additional allowed repositories + allowed-events: [COMMENT] # Preferred default for non-blocking bot reviews + supersede-older-reviews: true # Best-effort dismissal of older same-workflow REQUEST_CHANGES reviews (including legacy blockers) footer: "always" | "none" | "if-body" # Footer on review body ``` diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 71d14b20326..690e638b961 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -668,12 +668,17 @@ safe-outputs: target: "triggering" # or "*", or e.g. ${{ github.event.inputs.pr_number }} when not in pull_request trigger target-repo: "owner/repo" # cross-repository: submit review on PR in another repo allowed-repos: ["org/repo1", "org/repo2"] # additional allowed repositories - allowed-events: [COMMENT, REQUEST_CHANGES] # restrict allowed review event types (default: all allowed) + allowed-events: [COMMENT, REQUEST_CHANGES] # include REQUEST_CHANGES when using supersede mode for blocking reviews + supersede-older-reviews: true # dismiss older same-workflow REQUEST_CHANGES reviews after posting a replacement review footer: false # omit AI-generated footer from review body (default: true) ``` Use `allowed-events` to restrict which review event types the agent can submit. This provides infrastructure-level enforcement — for example, `allowed-events: [COMMENT, REQUEST_CHANGES]` prevents the agent from submitting APPROVE reviews regardless of what the agent attempts to output. If omitted, all event types (APPROVE, COMMENT, REQUEST_CHANGES) are allowed. +**Recommendation:** prefer `allowed-events: [COMMENT]` as the default for automated review workflows. This keeps AI feedback visible without creating a persistent merge-blocking state. + +Set `supersede-older-reviews: true` only when your workflow intentionally uses `REQUEST_CHANGES` and you want newer runs to dismiss older blocking reviews from the same workflow. Superseding is best-effort and happens after the replacement review is posted. + ### Resolve PR Review Thread (`resolve-pull-request-review-thread:`) Resolves review threads on pull requests. diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index f5e55bc61c2..06bcedfe8ea 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -6052,6 +6052,10 @@ "description": "Optional list of allowed review event types. If omitted, all event types (APPROVE, COMMENT, REQUEST_CHANGES) are allowed. Use this to restrict the agent to specific event types, e.g. [COMMENT, REQUEST_CHANGES] to prevent approvals.", "minItems": 1 }, + "supersede-older-reviews": { + "type": "boolean", + "description": "When true, after posting a replacement review this workflow dismisses older REQUEST_CHANGES reviews previously posted by the same workflow on the same pull request. This is best-effort and requires workflow markers in prior review bodies." + }, "github-token": { "$ref": "#/$defs/github_token", "description": "GitHub token to use for this specific output type. Overrides global github-token if specified." diff --git a/pkg/workflow/compiler_safe_outputs_handlers.go b/pkg/workflow/compiler_safe_outputs_handlers.go index ea53107c906..323799e839c 100644 --- a/pkg/workflow/compiler_safe_outputs_handlers.go +++ b/pkg/workflow/compiler_safe_outputs_handlers.go @@ -337,6 +337,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddIfNotEmpty("target-repo", c.TargetRepoSlug). AddStringSlice("allowed_repos", c.AllowedRepos). AddStringSlice("allowed_events", c.AllowedEvents). + AddIfTrue("supersede_older_reviews", c.SupersedeOlderReviews). AddIfNotEmpty("github-token", c.GitHubToken). AddStringPtr("footer", getEffectiveFooterString(c.Footer, cfg.Footer)). AddIfTrue("staged", c.Staged). diff --git a/pkg/workflow/js/safe_outputs_tools.json b/pkg/workflow/js/safe_outputs_tools.json index cd6f246f6e8..633ab635113 100644 --- a/pkg/workflow/js/safe_outputs_tools.json +++ b/pkg/workflow/js/safe_outputs_tools.json @@ -413,7 +413,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": { diff --git a/pkg/workflow/submit_pr_review.go b/pkg/workflow/submit_pr_review.go index fc75fc40749..09de47fe843 100644 --- a/pkg/workflow/submit_pr_review.go +++ b/pkg/workflow/submit_pr_review.go @@ -15,8 +15,9 @@ var submitPRReviewLog = logger.New("workflow:submit_pr_review") type SubmitPullRequestReviewConfig struct { BaseSafeOutputConfig `yaml:",inline"` SafeOutputTargetConfig `yaml:",inline"` - Footer *string `yaml:"footer,omitempty"` // Controls when to show footer in PR review body: "always" (default), "none", or "if-body" (only when review has body text) - AllowedEvents []string `yaml:"allowed-events,omitempty"` // Optional list of allowed review event types: APPROVE, COMMENT, REQUEST_CHANGES. If omitted, all event types are allowed. + Footer *string `yaml:"footer,omitempty"` // Controls when to show footer in PR review body: "always" (default), "none", or "if-body" (only when review has body text) + AllowedEvents []string `yaml:"allowed-events,omitempty"` // Optional list of allowed review event types: APPROVE, COMMENT, REQUEST_CHANGES. If omitted, all event types are allowed. + SupersedeOlderReviews bool `yaml:"supersede-older-reviews,omitempty"` // When true, dismisses older same-workflow REQUEST_CHANGES reviews after a replacement review is posted. } // parseSubmitPullRequestReviewConfig handles submit-pull-request-review configuration @@ -100,7 +101,15 @@ func (c *Compiler) parseSubmitPullRequestReviewConfig(outputMap map[string]any) } } - submitPRReviewLog.Printf("Parsed submit-pull-request-review config: max=%d, target=%s, target_repo=%s, allowed_events=%v", templatableIntValue(config.Max), config.Target, config.TargetRepoSlug, config.AllowedEvents) + if supersedeOlderReviews, exists := configMap["supersede-older-reviews"]; exists { + if supersedeEnabled, ok := supersedeOlderReviews.(bool); ok { + config.SupersedeOlderReviews = supersedeEnabled + } else { + submitPRReviewLog.Printf("Invalid supersede-older-reviews value: must be a boolean") + } + } + + submitPRReviewLog.Printf("Parsed submit-pull-request-review config: max=%d, target=%s, target_repo=%s, allowed_events=%v, supersede_older_reviews=%t", templatableIntValue(config.Max), config.Target, config.TargetRepoSlug, config.AllowedEvents, config.SupersedeOlderReviews) } else { // If configData is nil or not a map, set the default max config.Max = defaultIntStr(1) diff --git a/pkg/workflow/submit_pr_review_footer_test.go b/pkg/workflow/submit_pr_review_footer_test.go index 32422c1a455..8b6db8d56fd 100644 --- a/pkg/workflow/submit_pr_review_footer_test.go +++ b/pkg/workflow/submit_pr_review_footer_test.go @@ -279,6 +279,20 @@ func TestSubmitPRReviewFooterConfig(t *testing.T) { assert.Empty(t, config.AllowedEvents, "AllowedEvents should be empty when not configured") }) + t.Run("parses supersede-older-reviews field", func(t *testing.T) { + compiler := NewCompiler() + outputMap := map[string]any{ + "submit-pull-request-review": map[string]any{ + "max": 1, + "supersede-older-reviews": true, + }, + } + + config := compiler.parseSubmitPullRequestReviewConfig(outputMap) + require.NotNil(t, config, "Config should be parsed") + assert.True(t, config.SupersedeOlderReviews, "SupersedeOlderReviews should be parsed") + }) + t.Run("parses all three valid event types in allowed-events", func(t *testing.T) { compiler := NewCompiler() outputMap := map[string]any{ @@ -567,4 +581,44 @@ func TestSubmitPRReviewFooterInHandlerConfig(t *testing.T) { } } }) + + t.Run("supersede_older_reviews included in submit_pull_request_review handler config when true", func(t *testing.T) { + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "Test", + SafeOutputs: &SafeOutputsConfig{ + SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, + SupersedeOlderReviews: true, + }, + }, + } + + var steps []string + compiler.addHandlerManagerConfigEnvVar(&steps, workflowData) + require.NotEmpty(t, steps, "Steps should not be empty") + + stepsContent := strings.Join(steps, "") + require.Contains(t, stepsContent, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") + + for _, step := range steps { + if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { + parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ") + if len(parts) == 2 { + jsonStr := strings.TrimSpace(parts[1]) + jsonStr = strings.Trim(jsonStr, "\"") + jsonStr = strings.ReplaceAll(jsonStr, "\\\"", "\"") + var handlerConfig map[string]any + err := json.Unmarshal([]byte(jsonStr), &handlerConfig) + require.NoError(t, err, "Should unmarshal handler config") + + submitConfig, ok := handlerConfig["submit_pull_request_review"].(map[string]any) + require.True(t, ok, "submit_pull_request_review config should exist") + supersedeOlderReviews, hasSupersedeOlderReviews := submitConfig["supersede_older_reviews"].(bool) + require.True(t, hasSupersedeOlderReviews, "supersede_older_reviews should be in handler config when true") + assert.True(t, supersedeOlderReviews, "supersede_older_reviews should be true") + } + } + } + }) }